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

[bug] The root path is applied twice for editable packages #14079

Closed
fredizzimo opened this issue Jun 13, 2023 · 2 comments · Fixed by #14109
Closed

[bug] The root path is applied twice for editable packages #14079

fredizzimo opened this issue Jun 13, 2023 · 2 comments · Fixed by #14109
Assignees
Milestone

Comments

@fredizzimo
Copy link
Contributor

Environment details

Conan release/2.0 branch

Steps to reproduce

#13983 tries to fix editable packages with a root. But the fix does not quite work, since root is applied twice for the main node.

This can be seen by changing the test to

def test_editable_folders_root():
    """ Editables with self.folders.root = ".." should work too
    """
    c = TestClient()
    sub1 = textwrap.dedent("""
        from conan import ConanFile

        class Pkg(ConanFile):
            name = "pkg"
            version = "0.1"
            def layout(self):
                self.folders.root = ".."
                self.folders.build = "mybuild"
                self.cpp.build.libdirs = ["mylibdir"]

            def generate(self):
                self.output.info(f"PKG source_folder {self.source_folder}")
        """)
    sub2 = textwrap.dedent("""
        from conan import ConanFile

        class Pkg(ConanFile):
            requires = "pkg/0.1"
            def generate(self):
                pkg_libdir = self.dependencies["pkg"].cpp_info.libdir
                self.output.info(f"PKG LIBDIR {pkg_libdir}")
        """)
    c.save({"sub1/conanfile.py": sub1,
            "sub2/conanfile.py": sub2})
    c.run("editable add sub1")
    c.run("install sub2")
    libdir = os.path.join(c.current_folder, "mybuild", "mylibdir")
    assert f"conanfile.py: PKG LIBDIR {libdir}" in c.out
    assert f"conanfile.py: PKG source_folder {c.current_folder}" in c.out

The source folder becomes the parent of c.current_folder and the test fails.

The same thing can be observed when changing root to root for example, then all the paths gets root/root instead of just root

Here's the explanation of what happens:

conanfile.folders.set_base_folders(base_path, output_folder)
calls set_base_folders, which already sets the root as can be seen here
base_folder = conanfile_folder if self.root is None else \
os.path.normpath(os.path.join(conanfile_folder, self.root))
, so it gets applied twice.

When I was trying to fix the bug before I saw the fix in #1398 I just commented out the following lines (I have no idea why they would need to be there, and they caused the root to be set back to the wrong value)

conanfile.folders.set_base_package(output_folder or base_path)
conanfile.folders.set_base_source(base_path)
conanfile.folders.set_base_build(output_folder or base_path)
conanfile.folders.set_base_generators(output_folder or base_path)

Another thing that seems to work is to move

base_path = base_path if conanfile.folders.root is None else \
os.path.normpath(os.path.join(base_path, conanfile.folders.root))
down below the set_base_folders call.

But I wonder if that's really correct, should the root really be the same for all the nodes? I don't know the codebase and the Conan internals well enough to answer that.

Logs

No response

@AbrilRBS
Copy link
Member

AbrilRBS commented Jun 16, 2023

Hi @fredizzimo thanks a lot for your report, we appreciate it (Doing such a good investigation on your part proved extremely useful in tracking the issue down too 🫶)

You were spot on with the root cause too! I have simplified the code a bit and will add some more tests to this section before merging :)

@memsharded memsharded added this to the 2.0.7 milestone Jun 16, 2023
@memsharded
Copy link
Member

Closed by #14109, will be in 2.0.7 soon, thanks again for reporting!

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

Successfully merging a pull request may close this issue.

3 participants