-
Notifications
You must be signed in to change notification settings - Fork 1k
Fix Conan Source folders.root issue #18377
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
base: develop2
Are you sure you want to change the base?
Fix Conan Source folders.root issue #18377
Conversation
Thanks for your contribution.
Many thanks! |
- Fixed issue in local.py where source() method did not account for self.folders.root (when specified) - Added test/integration/command/source_test.py::SourceTest::test_local_source_layout_root Close conan-io#18376
8010531
to
72a1aa6
Compare
Added a test that verifies the source method applies the root when called locally |
class ConanLib(ConanFile): | ||
|
||
def layout(self): | ||
self.folders.root = "root_folder" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From docs in: https://docs.conan.io/2/reference/conanfile/methods/layout.html
self.folders.root (Defaulted to None): Specifies a parent directory where the sources, generators, etc., are located specifically when the conanfile.py is located in a separated subdirectory. Check this example on how to use self.folders.root.
So it seems this layout is not allowed. The self.folders.root
is intended for solving other issues, like the possibility of having the conanfile.py
inside a subfolder instead of having it in the root of the repo.
What problem are you trying to solve with this layout?
Sorry I didn't realized it in the ticket #18376, I'll put a note there, and maybe conversation can follow there too.
client.save({CONANFILE: conanfile}) | ||
|
||
client.run("source .") | ||
self.assertTrue(client.out.strip().endswith("spaces\\root_folder")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is most likely broken in other systems such as Linux, it seems you developed and tested it under Windows?
In any case, lets go back to the issue, to clarify the goal and the intent, and then we will go back to the PR code, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. Thanks for taking the time to review. I did test under windows.
The example from the documentation fails
When you run the example you can see that the source()
method fails because it is looking for CMakeLists.txt in the conan
directory. You can prove this by moving CMakeLists.txt into the conan
directory and then conan source .
will run successfully.
Conditions
I did see in the documentation that it said parent folder, but this fails even if following the parent folder guidance as mentioned above. The particular conditions under which this really matters:
- The conanfile is not stored in the same repository as the source
- You cannot use
export_sources()
because the source code should not be available to all package consumers no_copy_source=True
is used because the size of the repository is significant and the copy time is non_trivial (thus even in thebuild()
methodself.source_folder
must be used)
Failure when using parent folder
For example assuming a folder structure as follows:
Consider the following conanfile (which conforms to the parent folder guidance):
from conan import ConanFile
from conan.tools.files import save, load
class BasicConanfile(ConanFile):
name = "conan_source_bug"
version = "0.1"
description = "Demonstrate a bug in conan source() method"
no_copy_source = True
def layout(self):
self.folders.root = ".."
self.folders.source = "src"
self.folders.generators = "generated"
def generate(self):
save(self, "generated_file.txt", "content")
def source(self):
# Saves source to source_folder
self.output.info(f"In the source() method the Source folder is: {self.source_folder}")
save(self, "source_file.c", "content") #i.e. get source from somewhere
def build(self):
self.output.info(f"In the build() method the Source folder is: {self.source_folder}")
load(self, f"{self.source_folder}/source_file.c") # i.e. a compiler looking for source files
If you attempt to run conan source -> conan install -> conan build it will fail because the build method has a different definition of source_folder.
Correct me if I'm misunderstanding something.
Use case for having a root that is a subdir instead of parent
We ran across a use case where it t was desired that the source()
method and all other methods use a root that is a subfolder of the current directory. This is our preferred workspace setup:
- ParentDir
- conanfile.py
- root_folder
- src
- generated
From a unix "principle of least surprise" perspective, it seems root folder should function consistently whether it is a subdirectory or parent folder and I don't think it requires any additional code to support both cases.
The fix I submitted is agnostic to whether it's a parent or child folder (the above conanfile will also work with the submitted fix).
Let me know if I've missed anything!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please ask for guidance or help with the test if necessary. Thanks again for your contribution!
Tagging this for next 2.18 release
client.save({CONANFILE: conanfile}) | ||
|
||
client.run("source .") | ||
self.assertTrue(client.out.strip().endswith("spaces\\root_folder")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better to make the recipe read a file in the source() and build() method in the test, instead of printing the source folder. If those methods fail to find the file, then it will fail raising an error, I think that test will be easier than having to check the source folder path.
@@ -142,6 +142,30 @@ def source(self): | |||
self.assertIn("conanfile.py: Running source!", client.out) | |||
self.assertEqual("Hello World", client.load("file1.txt")) | |||
|
|||
def test_local_source_layout_root(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'd change this test to implement the recipe that you provided, with the self.folders.root = ".."
. That helps to tag this as a Bugfix
and be able to move it forward.
Then, I'd move new tests to cover the use case of a child "root" folder to a different PR, to be done later, once this is merged.
Close #18376
Changelog: (Bugfix): Fixes issue where conan source() method doesn't use folders.root when present
Docs: https://docs.conan.io/2/reference/conanfile/methods/layout.html#self-folders
No doc update should be needed as this implements the expected behavior for folders.root.