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] self.package_folder is None in package_info() if layout() is used #12310

Closed
elqueffo opened this issue Oct 14, 2022 · 25 comments · Fixed by #12318
Closed

[bug] self.package_folder is None in package_info() if layout() is used #12310

elqueffo opened this issue Oct 14, 2022 · 25 comments · Fixed by #12318

Comments

@elqueffo
Copy link

Environment Details (include every applicable attribute)

  • Conan version: 1.52.0, 1.53.0
  • Python version: 3.10

Steps to reproduce (Include if Applicable)

Create a conan file like so:

    def package_info(self):
        bin_path = os.path.join(self.package_folder, "bin")

    def layout(self):
        cmake_layout(self)

Set package in editable mode

install with CMakeDeps and CmakeToolchain generator:

conan install package/0.1@blah/blah -g CMakeDeps -g CMakeToolchain

Logs (Executed commands with output) (Include/Attach if Applicable)

ERROR: package/0.1@blah/blah : Error in package_info() method, line 228
        bin_path = os.path.join(self.package_folder, "bin")
        TypeError: expected str, bytes or os.PathLike object, not NoneType
@memsharded
Copy link
Member

Hi @elqueffo

I have been checking, but it seems there might be something else in your recipe affecting. This is what I tried to reproduce:

  • Start from conan new hello/0.1 -m=cmake_lib
  • Added that line using self.package_folder to conanfile.py package_info() method
  • Did a conan create . (the test_package that consumes it already uses CMakeDeps and CMakeToolchain
  • Did a conan install hello/0.1@ -g CMakeToolchain -g CMakeDeps, seems to work too.

So if you could please provide a fully reproducible example, that would help. Thanks!

@elqueffo
Copy link
Author

Hey @memsharded

Here is a full conanfile:

from conans import ConanFile
from conan.tools.layout import cmake_layout
import os

class TestConan(ConanFile):
    name = "test"
    version = "0.9.1"
    description = "test"
    license = "MIT"
    topics = ("test")
    homepage = "https://test"
    url = "https://test"
    short_paths = True

    settings = "os", "arch", "compiler", "build_type"

    generators = "cmake", "cmake_find_package"

    def source(self):
        print("source")

    def build(self):
        print("build")


    def package(self):
        print("package")

    def package_info(self):
        print("package_info")

        bin_path = os.path.join(self.package_folder, "bin")
        self.output.info("Appending PATH env var with: {}".format(bin_path))
        self.env_info.PATH.append(bin_path)
    
    def layout(self):
        print("layout")
        cmake_layout(self)

I run these commands:

  • conan create . test/test
  • conan editable add . test/0.9.1@test/test
  • conan install test/0.9.1@test/test -g CMakeDeps -g CMakeToolchain

the output i get is as follows:

Configuration:
[settings]
arch=x86_64
arch_build=x86_64
build_type=Release
compiler=Visual Studio
compiler.runtime=MD
compiler.version=16
os=Windows
os_build=Windows
[options]
[build_requires]
[env]

layout
Installing package: test/0.9.1@test/test
Requirements
    test/0.9.1@test/test from user folder - Editable
Packages
    test/0.9.1@test/test:3fb49604f9c2f729b85ba3115852006824e72cab - Editable

Installing (downloading, building) binaries...
package_info
ERROR: test/0.9.1@test/test: Error in package_info() method, line 32
        bin_path = os.path.join(self.package_folder, "bin")
        TypeError: expected str, bytes or os.PathLike object, not NoneType

If i remove layout() , i can successfully run install.

@memsharded
Copy link
Member

Sorry, I accidentally closed the issue instead of commenting on it. Re-opening.

The missing detail was the editable thing, that is very important. I will try to reproduce with it, thanks.

@memsharded memsharded reopened this Oct 15, 2022
@memsharded
Copy link
Member

Some deprecation warnings:

  • generators = "cmake", "cmake_find_package". These generators have been superseded by CMakeDeps and CMakeToolchain in 1.X, and have been removed in 2.0. please upgrade to those new ones to be 2.0-ready (which is getting very close)
  • Same for self.env_info.PATH has been replaced by self.buildenv_info.define_path("MYVAR", mypathvalue) and the VirtualBuildEnv generator. Actually for the bin folder inside the package, it is no necessary to do it explicitly, the VirtualBuildEnv generator will manage.

@memsharded
Copy link
Member

I have been able to reproduce.
This is kind of expected. When a package is in editable mode its self.package_folder is None, it is not defined because there is no package at all, this cannot have a value.
So in order to define a package_info() that is valid for editable, it needs to be based on relative paths. This is what the cpp_info.includedirs = ["include"] and similar is already doing. Given the comment above in the deprecation warning, that it is actually not necessary to add the bin folder to the PATH env-var, as it will be managed automatically by VirtualBuildEnv, maybe this wouldn't be an issue anymore?

@elqueffo
Copy link
Author

elqueffo commented Oct 17, 2022

There are numerous recipes using package_folder on conan-center and elsewhere, and it is the recommended/documented way to set path: https://docs.conan.io/en/latest/reference/conanfile/methods.html#env-info

I would therefor expect this to work also for editable pacakges in some way, with a "portable" way to set env vars based on the package location that works in both cases.

@santhonisz
Copy link
Contributor

I have also encountered this issue when using editable packages and the MSBuildDeps generator - you can see that it is explicitly attempting to write the package_folder to the generated props file and fails as it is None.
https://github.com/conan-io/conan/blob/develop/conan/tools/microsoft/msbuilddeps.py#L168

The workaround I've found is to call self.folders.set_base_package('.') in the layout method.

@memsharded
Copy link
Member

@elqueffo

There are numerous recipes using package_folder on conan-center and elsewhere, and it is the recommended/documented way to set path: https://docs.conan.io/en/latest/reference/conanfile/methods.html#env-info

Many recipes in ConanCenter haven't been migrated to the new tools, and in any case it is ok if they are not prepared to work in editable mode. Editable mode is when you want to be live editing the source code of several packages simultaneously, which is not a case for third party open source dependencies.

I would therefor expect this to work also for editable pacakges in some way, with a "portable" way to set env vars based on the package location that works in both cases.

That is the thing, it is not that we don't want to define the self.package_folder value, is that this value doesn't exist at all, we cannot set it to anything because the os.path.join(self.package_folder, "bin") folder will not exist anyway, for any value of package_folder, when the package is in editable mode. So the approach so far is:

  • If you use the cpp_info.xxxdirs variables, they are defined as relative paths, and also have their model in layout() with the self.cpp.build.xxxdirs variables and work in both contexts.
  • For cpp_info.set_property() properties, Conan only handles the cmake_build_modules one so far.

It makes sense to extend or provide some new mechanism that allows to transmit this information in a way that works in both cases, and this is the reason I was asking for further feedback about the real use cases (the case presented above for adding the "bin" folder to the PATH is already managed automatically), because that would allow to define the solutions better.

The solution proposed by @santhonisz with The workaround I've found is to call self.folders.set_base_package('.') in the layout method. is a bit dangerous, because it is not documented and can break at any time, but still, it won't be able to work in cases like the above that hardcode the "bin" folder path in an environment variable.

@elqueffo
Copy link
Author

I see, thanks for the explanation.

In this case this was in fact an open source dependency that i wanted to add a layout to for iterating faster. I wasn't able to without breaking a number of other things along the way, this being one of them.

For this specific recipe, i would have expected the bin path to point at the build output folder.

In addition this usecase:
On our own recipes we set a number of variables in user_info, using package_folder as base, and then parse those back from the conan build info json (deps_user_info) for use in our own tooling. I didn't write this part of our tooling myself, so maybe there is a way to derive the absolute path using relative paths there as well?

@memsharded
Copy link
Member

For this specific recipe, i would have expected the bin path to point at the build output folder.

It is. The bindirs is pointing to os.path.join(self.package_folder, "bin") for the case it is a package in the cache, and to the os.path.join(self.build_folder, self.cpp.build.bindirs[...]) in case it is a package in editable mode. You can access its value with self.dependencies["mydep"].cpp_info.bindirs (or any other xxxdirs), and it will be the correct path.

So the question is: what needs to define a folder inside the package_folder that isn't a "bin", isn't a "lib", isn't a "include" header, isn't a resource "res"?

@memsharded
Copy link
Member

Hi @elqueffo

Any feedback regarding the last questions? PR #12318 is scheduled to close this ticket, as not a bug, but expected behavior, package_folder will be None if package is in editable mode, and if packages are to be used in editable mode, they shouldn't rely on package_folder at all, but on the cpp_info abstractions.

@elqueffo
Copy link
Author

elqueffo commented Nov 2, 2022

Hello again @memsharded ,

I am not sure i follow how i would achieve this still:

Inside package_info() how could i set some arbitrary environment variable self.env_info.BOB or user info self.user_info.ALICE to an absolute path of an arbitrary file inside the package (that would work in both editable and normal mode)?

I understand some generators already handle setting PATH, but what about other custom variables like this?

@memsharded
Copy link
Member

Inside package_info() how could i set some arbitrary environment variable self.env_info.BOB or user info self.user_info.ALICE to an absolute path of an arbitrary file inside the package (that would work in both editable and normal mode)?

This is what I am trying to understand: what is "BOB" and what is "ALICE"? It would be good to learn about the real use cases, so we consider alternative ways to model this. Certainly self.package_folder is not possible in editable mode, as it doesn't exist at all, but there are some abstractions over that that could help, like the cpp_info one, or the properties, or maybe we need some other mechanism to express relative paths instead of absolute ones. For example, if the specific files are resources, you already have access in the user side as cpp_info.resdir, and knowing only the filename would be enough to use that from consumers.

@memsharded memsharded added this to the 1.55 milestone Nov 2, 2022
@memsharded memsharded reopened this Nov 2, 2022
@memsharded
Copy link
Member

Re-opening after #12318 was merged and closed it, and assigning for discusion to next 1.55:

  • We need to check the MSBuildDeps generator, it shouldn't fail when editable because of accessing package_folder, I think this was already fixed in CMakeDeps, but this was missing
  • We need to learn about real use cases that involve knowing paths of things that should be used both in editable and in normal package, and might not be covered by cpp_info

@elqueffo
Copy link
Author

elqueffo commented Nov 2, 2022

This is what I am trying to understand: what is "BOB" and what is "ALICE"?

Let's say i am packaging some library or tool that expects a specific environment variable to be set to point to a config file, that resides inside the package folder.

having access through cpp_info wouldn't help here i think?`

Let's say MyPackage expects MY_PACKAGE_CFG to be set to an absolute path to my_package.ini, could i do that based on cpp_info? Or perhaps VirtualEnv generator would handle relative to absolute on arbitrary env vars?

@memsharded
Copy link
Member

Let's say i am packaging some library or tool that expects a specific environment variable to be set to point to a config file, that resides inside the package folder.

Where does it reside when the package is in editable mode, in the source&build tree? It is a generated file, or is it part of the source?

@memsharded
Copy link
Member

In any case, I am starting to understand the use case, and this seems not a bug, but a request for a new feature that helps managing some use cases, but this feature might need some thinking about the right model, and how to represent the different situations between the editable layout (source & build trees) vs the package layout (only package tree)

@elqueffo
Copy link
Author

elqueffo commented Nov 2, 2022

Where does it reside when the package is in editable mode, in the source&build tree? It is a generated file, or is it part of the source?

In this case it would be in the source tree, however it would probably be useful to also support the case where it is part of the build output.

@memsharded
Copy link
Member

Ok, so this is the requirement:

  • We need to define a way, to specify a environment variable in an abstract way, so it can point to a file inside the build tree in editable layout and to a file inside the package in cache layout, so it gets the full correct path in the consumer side.

We certainly need to think about this, doesn't seem an easy feature, as it requires some similar generalization and management like the one happening for cpp_info.

@santhonisz
Copy link
Contributor

  • We need to check the MSBuildDeps generator, it shouldn't fail when editable because of accessing package_folder, I think this was already fixed in CMakeDeps, but this was missing

For our particular use case, this is this current issue that is affecting us. At present we can't use packages in editable mode if they use the MSBuildDeps generator (unless we use the work-around I previously suggested, but you understandably recommend against)

@paulharris
Copy link
Contributor

Another example is if I need to hack on QT (eg bugfixes, new features, etc),
I would have my primary app that uses QT, and I'd want to put the QT library into editable mode, hack and compile QT.
Then I'd want to build my primary app and it would use the editable QT.

QT has this QT_PLUGIN_PATH which is key to executing QT programs.
It points to a folder in the package folder.

I have no idea where those files might exist in the build folder - they are possibly scattered across the build, and only collected up during packaging.

I would expect to have to do a kind of loop when editing QT sources:

  • edit qt code
  • compile
  • run the package to collect/install the dlls, headers, plugins into the package folder.

This is the loop I used to run prior to my entry into conan... something like

cd somelibrary
vim sourcefiles
ninja -C ../build_somelibrary
ninja -C ../build_somelibrary install

Where make-install would deploy to build_somelibrary/install

The make-install step is handy because you may not want to replace the deployed libraries/headers after every make (ie work in progress, build not complete, only half of the libraries recompiled in build folder).

Maybe you can ask the consumer programs to link to the dlls and headers wherever they are in the build folder,
but that may result in #include finding all sorts of internal files that isn't typically deployed .... Seems like way too hard to me.

@memsharded
Copy link
Member

memsharded commented Nov 8, 2022

But @paulharris this is the key:

run the package to collect/install the dlls, headers, plugins into the package folder.

If you already need to run an extra package command, why would you need the editable feature at all? You could do a conan export-pkg, and it will be a regular Conan package in the cache, no editable, and everything will work smoothly. Any potential benefit of build systems using directly the product of the build of the editable are completely lost with that copy produced by the package. Also the time for running conan package and conan export-pkg should also be identical, so no performance/speed issue here either.

@paulharris
Copy link
Contributor

Good point, I guess I can't say for certain without trying the feature in its full v2 glory,

\So for my example, how would the consumer be able to find the QT_PLUGINS_PATH and its generated dlls, when the package is in editable mode? ie without the packaging step?

@memsharded
Copy link
Member

memsharded commented Nov 16, 2022

Hi all,

The minor issues like MSBuildDeps failing have been solved in #12529

The main remaining issue is a very complex one, which is the definition of env-vars, we have created a explicit ticket for it: #12499. We can follow that one, and I will close this one (as the title is a bit misleading, self.package_folder is None if editable by definition, not a bug).

The bad news is that at the moment solving #12499 is not obvious, and it seems pretty challenging. That means that in the general case, it is not possible to put such packages in editable mode that defines env-vars with paths involving package_folder.

The workaround would be avoiding the editable for this case: as the editables are actually for packages which source code is being edited, and not the case in the vast majority of cases for third-party open source, then rely on the local flow build + conan export-pkg instead, rather than on editables

Closing this, please follow #12499 (likely to be addressed later in 2.X). Many thanks!

@paulharris
Copy link
Contributor

Seem, seems to have been fixed by #12598 - awesome!

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.

4 participants