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

Add example of editable package with custom src folder layout #156

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

paulharris
Copy link

@paulharris paulharris commented Jul 19, 2024

Please consider adding a more complicated example,
where the user can use cmake_layout() AND add to the layout() function to support editable, where the source is in an unusual layout.

Also, this example demonstrates how to use the consumer to initialise the editable dependency build environment, which is required when the consumer may apply a complex set of options to the dependency.

I hand-tested the ci_test_example.py script, but not sure if it works on the CI.
Feel free to improve, but note my goal is to improve conan documentation and examples. It isn't helpful when the documentation and examples only cover the extremely simple (cmake_layout is enough) and extremely complex (everything is customised).

Also gives an example where the consumer applies additional options to
the dependency library.
@CLAassistant
Copy link

CLAassistant commented Jul 19, 2024

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@memsharded memsharded left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution @paulharris

I think overall it can be a good addition, just some comments:

  • It shouldn't be part of the main "tutorial" section, but an "example" that can be linked from the tutorial section.
  • ci_test_example can be simplified a bit to build only 1 config, I think 1 config demostrates enough the custom layout
  • Things not related to the custom layout can be removed.

Comment on lines +21 to +24
def validate(self):
# ensure the consumer's options affect this dependency
if not self.options.something_custom:
raise ConanInvalidConfiguration("The consumer should set something_custom=True")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This validate() seems unrelated to the example, I'd say it should be simplified and removed, and the example focused on the custom layout.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The point of validate() was to ensure the build of "say" had the flag set. That flag was set by the consumer.
The original example built "say" without any influence from the consumer.
If you attempted to build this example with the same procedure as the other example (ie conan-install "say" directly), then it should fail.
Perhaps that should be a different example?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re simplification, I got rid of the Debug build. It was there in the original example.
Can I also get rid of the "beta9" stuff?
I didn't test on Windows, do I really need the cmake --preset conan-default windows specialisation?
Seems weird that it is different on Windows.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you give me a place where it should be moved?
Are you able to help with the HTML linking?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The point of validate() was to ensure the build of "say" had the flag set. That flag was set by the consumer.

I might still be missing something. The option is not used at all for the build, is it? If it doesn't have any effect on the layout, the build or anything, it still doesn't seem related to an example that is focused on the custom source folder. This would be a completely different use case, which is setting options from consumers, but this still doesn't seem related to the current example of the layout().

cmake_layout(self)

self.cpp.source.includedirs = ["thelib/src"]
self.cpp.build.libdirs = ["thelib/src"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem a valid cpp.build.libdirs, I don't think the final say.lib goes in thelib/src/say.lib.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It sure does.

$ ls build_say/build/Release/thelib/src/
CMakeFiles  cmake_install.cmake  libsay.a

This matches the way a real library is built.
The source is not at the top level, there are several directory levels with documentation and other things before it gets to the source, and that is where cmake builds the library.

A key thing that is demonstrated here is the libraries will end up in a very different path to after it is packaged.
ie after packaging, it is in the sensible package/lib folder. Before, libraries may be getting built in all sorts of places.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@paulharris keep in mind that people use different platforms and generators. cmake does do different build folder layouts depending on that.

i've been using the following expression usually:

# this sets self.cpp.build.libdirs[0] (and bindirs) to a reasonable value based on build context
# it usually becomes "." or self.build_type
cmake_layout(self)

# example below expects something like
# /CMakeLists.txt -> add_subdirectory(relative/path/to/add_library/call)
# /relative/path/to/add_library/call/CMakeLists.txt -> add_library(foo)
self.cpp.build.libdirs = [
  os.path.join("relative", "path", "to", "add_library", "call", self.cpp.build.libdirs[0])
]
self.cpp.build.bindirs = [
  os.path.join("relative", "path", "to", "add_library", "call", self.cpp.build.bindirs[0])
]
self.cpp.build.libs = ["foo"]

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

Successfully merging this pull request may close these issues.

4 participants