Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Design Recommendation for Workspaces and Editable #4142

Closed
3 tasks done
solvingj opened this issue Dec 14, 2018 · 7 comments
Closed
3 tasks done

Design Recommendation for Workspaces and Editable #4142

solvingj opened this issue Dec 14, 2018 · 7 comments
Assignees
Labels
whiteboard Put in common
Milestone

Comments

@solvingj
Copy link
Contributor

solvingj commented Dec 14, 2018

In order to support user-defined workspace layouts, the conan install command should simply take more parameters when called with --editable mode, similar to --install-folder . An example would be:

conan install . bincrafters/testing --editable -pr msvc_15_x64_release --user_libdir build_out/1 --user_libdir build/out/2 --user_includedir include

And then, support them in profiles:

conan create . bincrafters/testing --editable -pr msvc_15_x64_release_editable
	msvc_15_x64_release_editable

		include(msvc_15_x64_release)
		[user_libdirs]
		build_out/1
		[user_includedirs]
		build_out/2

The above profile example is one that would probably live in the user's .conan/profiles directory because it specifies the users "personalized" layout which they are likely to use on most of their projects. Compared to the current suggestions around requiring a new file in each repository, this features much less redundant copy and pasting for the common case.

However, sometimes they may need to deviate for one project or another, in which case they would want to put the definitions in a file inside the repository directory. This is already well supported by Conan.

conan create . bincrafters/testing --editable -pr ./msvc_15_x64_release_editable
	msvc_15_x64_release_editable_project_a

		include(msvc_15_x64_release)
		[user_libdirs]
		build_out/1
		[user_includedirs]
		build_out/2

Profiles are an already well-defined specification, which elegantly support user customizations, as well as project customizations that can live in the project directories. This use of profiles for passing user-defined paths even seems to align with the nature of profiles well from a philosophical perspective. It doesn't feel like a misuse of the feature intended for something else, it feels right to me.

Of note, this approach would benefit significantly by the implementation of the following feature request, which makes profiles more composable.

#4141

Most importantly, I feel strongly that editable and workspaces do not need a separate file specification with complicated logic to define their personal or organizational conventions for local directory layouts. Use cases for editable generally fall into two categories:

  1. Independent users with personal conventions
  2. Organizations with shared conventions

The approach of requiring a new file in each repository has issues for both cases. This approach does not suffer from those issues, and is dramatically easier to implement.

For the record, my team is in the second group, and everything we do is based on profiles because it's the only way to manage consistency across the organization's users and CI. For the case of editable feature, we need a way to define the default layouts across all of our projects, but don't want to have to copy some template file to each repository. Also, for the exceptional projects, we need the ability to override the layout that does live with in the repository. Finally, we need a way to bypass both and define the values at the CLI. Profiles give us all of these things.

My first choice was to define these paths in a new function of conanfile called package_info_user(), and share that via python_requires. However, that has been rejected twice now and I can see it's not up for discussion. So, I feel this profiles based approach is equally elegant, perhaps moreso. It handles both the "independent" and "shared" use cases very well, and is extremely powerful. When compared to the idea of defining a new yaml file specifications, and a "new language" for defining conditional logic in directories, I think this approach should be vastly more desirable for both Conan team, and customers.

Feedback welcome!

  • I've read the CONTRIBUTING guide.
  • I've specified the Conan version, operating system version and any tool that can be relevant.
  • I've explained the steps to reproduce the error or the motivation/use case of the question/suggestion.
@jgsogo
Copy link
Contributor

jgsogo commented Dec 15, 2018

Hi!

The main point here is that several packages in editable mode would like to share the same layout information, but it is not possible as we need to identify which sections affects to which reference.
Right now the implementation requires a file per library to override the cpp_info dictionary with the paths to the resources in the user workspace (includedirs, libdirs, resdirs, bindirs), this file affects only to the library that declares it.

Having a profile-like approach would require to specify which packages should be affected by which declarations. So the file will be more like:

	msvc_15_x64_release_editable_workspace

		include(msvc_15_x64_release)
		
                [project_a:libdirs]
		build_out/1
		[project_a:includedirs]
		build_out/2

		[project_b:libdirs]
		build_out/1

		[project_c:includedirs]
		build_out/2

and Conan will take into account only the projects that are currently in the editable mode.

profile-like approach:

  • no need for an additional file in each package to install it as editable
  • extra file to share (easy inside a company)
  • extra file to maintain (outside the repo), and to modify/version if new projects are added or modified
  • if installed as editable, and the profile-file is not provided (or the information related to the reference is missing), it will be an error.

file next to conanfile.py:

  • the author/developer of the package is the one who knows the project layout, it is the one that can better create the file. The layout of the project can change from one version to another, and this file will be up to date.
  • work out of the box (if the file is provided)
  • Conan can check for this file when the user installs a reference as editable
  • Can be contributed

I cannot see any real advantage for the profile-like approach. If you are already in a company I think it is easier to add the required file to the package itself, get that file versioned and it will be up to date all the time: the developer can install the references he wants in editable mode and it will work without needing to modify any profile file.
From the community point of view, the author of a package can provide that information (better in a file) and if not, the user must investigate the package to find this information and then write it in a file (profile) or in the repo (and make the PR to contribute it), I prefer the second option as it will make it available to everyone.

A different question is, does it makes sense to be able to override this information using a profile-like or through the command line? I'm not sure, I think we need more feedback about it.

Maybe I'm not understanding your use case, or I haven't suffered it and I cannot realize how important it is... I will think about it, and share it with the team.

Thanks!

@jgsogo jgsogo self-assigned this Dec 15, 2018
@jgsogo jgsogo added this to the 1.11 milestone Dec 15, 2018
@solvingj
Copy link
Contributor Author

Sorry, I should have covered this case. For the per-project definitions, we should be able to do just like options:

cd project_a
conan install . -e --user_libdir project_a:build_out/1
conan install . -e --user_libdir project_a:build_out/1
[user_libdirs]
project_a:./build_out/1
project_b:./build_out/2
*:./my_personal_default_convention_for_libdir

Maybe I'm not understanding your use case, or I haven't suffered it and I cannot realize how important it is... I will think about it, and share it with the team.

Yes, it's not your fault. I think this is the issue, and it shows how design-by-github-issue is ineffective. It's fine for reporting bugs and suggesting small changes, but it's painful, frustrating, and ineffective to try to discuss a new feature this way. It requires a huge body of text to provide all the context necessary to explain a situation. Yet, nobody wants to read huge body of texts in Github issues, and it's frowned upon.

In this case, I am the person who suggested/requested the editable feature to solve a real and present group of problems which I work with every-day, and which I have discussed and thought about with others who have the same or related needs for several months. Based on all this perspective and a lot of thought, I did my best to describe and negotiate a generalized feature which covers the known use-cases, and a wide range of related cases.

Somehow, despite this earnest attempt at communication on both sides, I feel like there is a major disconnect, and based on the way it's being planned right now, it does not look like something I would want to use for any of the cases it was originally suggested for.

@memsharded memsharded modified the milestones: 1.11, 1.12 Dec 18, 2018
@jgsogo
Copy link
Contributor

jgsogo commented Dec 24, 2018

Hi again,

We've changed a little bit the implementation of the editable package from an install perspective (#4152) to a link one (#4181), and we've been starting to use it internally to test the concept. I think that there are some changes that we need to introduce to make it more usable.

Now we are just linking a reference in the cache to a local workspace (an existing directory), nevertheless there are still some prerrequisites: the local workspace must contain a conanfile.py (I'm not sure if this is strictly needed, but let's keep it for now), and the local workspace must contain the magic .conan_package_layout file (read below).

The objective of the .conan_package_layout is/was to store the paths to the package dirs (includedirs, libdirs, resdirs, bindirs) so, anyone using the package would have them in the repo itself. Nevertheless we are talking about the user workspace and we know that the user loves to do things his/her way... so the paths in the .conan_package_layout won't be suitable for most of them, for sure. The package can use CMake, and given different generators I'm sure that the resulting paths won't be compatible, so we need to enable a mechanism for the user to override them and this is totally aligned with what you suggested in this issue. I was looking in a different direction in my previous comment 💩

I'm not sure about the name or where to locate a profile-like file, but editable packages need more flexibility, let's think about this fallback sequence:

  • user provided file in the cache, explicit params for each pacakge:
    [package_A:includedirs]
    my-custom-dir/{settings.os}/whatever
  • user provided file in the cache, wildcard for all packages
    [*:includedirs]
    my-custom-dir/{settings.os}/whatever
  • package provided file inside the repo, the .conan_package_layout file
    [includedirs]
    my-custom-dir/{settings.os}/whatever
  • conanfile.py provided directories in the package_info function, but relative to the workspace directory.

I'm going to implement this in a branch to test it, only one more consideration: first matching alternative for each package will be taken for all its directories, there won't be any composition/addition from all the possible sources.

Happy Christmas! 🎄

@jgsogo
Copy link
Contributor

jgsogo commented Jan 4, 2019

After sharing this with the team, we are moving towards changing the priority (from higher priority to lower):

  • information provided in a file inside the repo:
    [includedirs]
    my-custom-dir/{settings.os}/whatever
  • information provided in a file inside the cache:
    • explicit information
      [package_A:includedirs]
      my-custom-dir/{settings.os}/whatever
    • wildcard one:
      [*:includedirs]
      my-custom-dir/{settings.os}/whatever
  • conanfile.pyprovided directories in the package_info function, but relative to the local directory.

This is more aligned with my first comment, although I may have some drawbacks:

  • at this moment there is no way for the user to skip the file inside the package (only deleting it) (we are considering adding an additional argument to the conan link command in order to provide a path to the file with information).
  • paths defined in this file may not be suitable for all the build_systems, generators,... that the user can create to build the package locally. The list of directories can grow forever.

I'm going to change the PR #4181 to address these last comments, but I think it is worth to read all this thread again (ping @conan-io/barbarians) before accepting the final draft. Thanks for the feedback.

@jgsogo
Copy link
Contributor

jgsogo commented Jan 25, 2019

Given the evolution of the editable feature and the decision finally adopted, I think that this issue can be closed until we get actual feedback from users regarding this feature. I'm sure there will be some things to evolve and to improve.

@jgsogo
Copy link
Contributor

jgsogo commented Jan 28, 2019

Feedback on this feature should consider already merged work in #4287 and documented in conan-io/docs#1009. It will be released as experimental in v1.12 during this week.

Thanks, and, as always, we will be pleased to hear your valuable feedback.

@jgsogo jgsogo closed this as completed Jan 28, 2019
@ghost ghost removed the stage: triaging label Jan 28, 2019
@solvingj
Copy link
Contributor Author

you guys are really awesome

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
whiteboard Put in common
Projects
None yet
Development

No branches or pull requests

3 participants