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

fix cwd issues in set_version() evaluation #6130

Merged

Conversation

@memsharded
Copy link
Member

memsharded commented Nov 25, 2019

Changelog: Bugfix: Make sure set_version() runs in the conanfile.py folder, not in the current folder, so relative paths are not broken if executing from a different location.
Docs: conan-io/docs#1490

Close: #6129

@memsharded memsharded requested review from danimtb and jgsogo Nov 25, 2019
@memsharded memsharded added this to the 1.21 milestone Nov 25, 2019
Copy link
Member

danimtb left a comment

I think it is consistent with the behavior in current commands

Copy link
Member

jgsogo left a comment

I'm not sure about this change. Before you were able to use os.getcwd() (wherever the conan export was called from) or os.path.dirname(__file__) if you really want to build a path starting from the conanfile.py.

With this change we've lost the first alternative.... and we've just won the ability to be less explicit about the path.


I'm aware about the python-requires issue, but it can be worked around passing the os.path.dirname(__file__) to the function (copied from #6129):

base_recipe = python_requires("bundle_conanfile/1.0@asaljo/testing")

class Builder(base_recipe.get_conanfile()):
  name = "test"
  version = base_recipe.get_bundle_version(os.path.dirname(__file__))
@memsharded

This comment has been minimized.

Copy link
Member Author

memsharded commented Nov 25, 2019

I'm aware about the python-requires issue, but it can be worked around passing the os.path.dirname(file) to the function

That is part of the issue, we don't want to have a custom method/function, we want to inherit the set_version() method, and you cannot pass arguments to it. Take into accounts that python_requires are going to be move to a declarative interface, where the module is no longer available at recipe parse time.

@jgsogo

This comment has been minimized.

Copy link
Member

jgsogo commented Nov 26, 2019

IMO it is never a good idea to change the current-working-folder. Why not use the same strategy as we do for the source_folder or the build_folder and provide a xxxxx_folder which is the path to the folder where the recipe being built is located? It would fulfill the requirement too.

@memsharded

This comment has been minimized.

Copy link
Member Author

memsharded commented Nov 26, 2019

IMO it is never a good idea to change the current-working-folder. Why not use the same strategy as we do for the source_folder or the build_folder and provide a xxxxx_folder which is the path to the folder where the recipe being built is located? It would fulfill the requirement too.

Conan already changes the working directory for other methods, like source(), build() and package(). This is nothing new, has been there since always.

For a python_require, it is not possible to use something like:

def set_version(self):
     self.version = load(os.path.join(self.recipe_folder, "version.txt")

Because that self.recipe_folder would still be the folder of the python_require, not the consumer of the python_require. Python requires are shared among their consumers too.

I still need to figure out a use case in which the result of conan export or conan create will be different depending on your current location. As I see it, the behavior of the recipe should be completely invariant, the result should always be the same if you do conan export pkg_folder, or conan export . or conan export path/to/myconanfile.py. If you want to inject version information from the outside of the recipe, you should probably skip the set_version() method and use conan export <path> version@user/channel form.

@jgsogo

This comment has been minimized.

Copy link
Member

jgsogo commented Nov 26, 2019

IMO it is never a good idea to change the current-working-folder. Why not use the same strategy as we do for the source_folder or the build_folder and provide a xxxxx_folder which is the path to the folder where the recipe being built is located? It would fulfill the requirement too.

Conan already changes the working directory for other methods, like source(), build() and package(). This is nothing new, has been there since always.

It doesn't mean it is a good idea to do it.

I don't come across many recipes with os.getcwd() (zero matches in conan-center-index) and I find many with things like self.souce_folder or self.build_folder.

For a python_require, it is not possible to use something like:

def set_version(self):
     self.version = load(os.path.join(self.recipe_folder, "version.txt")

Because that self.recipe_folder would still be the folder of the python_require, not the consumer of the python_require. Python requires are shared among their consumers too.

The self.recipe_folder should be the folder of the package being built, exactly the same as the self.source_folder or the slef.build_folder. If we use a python_requires like:

# Here it goes my python_requires recipe
class MyRecipe(ConanFile):
    ...

def get_conanfile():
    class BaseConan(ConanFile):

        def build(self):
            .... os.path.join(self.build_folder, "CMakeLists.txt")

it will use the build_folder of the package being built, it won't raise because a python_requires doesn't have the concept of build or package or binaries... and the build_folder doesn't make sense for it.

I still need to figure out a use case in which the result of conan export or conan create will be different depending on your current location. As I see it, the behavior of the recipe should be completely invariant, the result should always be the same if you do conan export pkg_folder, or conan export . or conan export path/to/myconanfile.py. If you want to inject version information from the outside of the recipe, you should probably skip the set_version() method and use conan export <path> version@user/channel form.

Totally agree, but let's do it with an explicit attribute that always has the same value rather than with a os.getcwd() call that returns different values depending on where it is written. That's my only point.


Full picture: explicit, no current working directory, lovely 😄

class MyRecipe(ConanFile):
    ...

def get_conanfile():

    class BaseConan(ConanFile):

        def set_version(self):
             self.version = load(os.path.join(self.recipe_folder, "version.txt")

        def build(self):
            .... os.path.join(self.build_folder, "CMakeLists.txt")
@memsharded

This comment has been minimized.

Copy link
Member Author

memsharded commented Nov 28, 2019

Yes, I see the value of self.version = load(os.path.join(self.recipe_folder, "version.txt"), lets make a decision @lasote to see if possible for 1.21

@memsharded memsharded requested a review from jgsogo Nov 29, 2019
@memsharded

This comment has been minimized.

Copy link
Member Author

memsharded commented Nov 29, 2019

Last changes contain the self.recipe_folder solution, but can be reverted if decided otherwise.

@jgsogo
jgsogo approved these changes Nov 29, 2019
Copy link
Member

jgsogo left a comment

I like it with the self.recipe_folder.

Not sure about the scope of the variable, but better to start with the minimal possible and expand it if needed.

@lasote
lasote approved these changes Dec 3, 2019
Copy link
Contributor

lasote left a comment

The docs are needed. We need to mention somewhere the recipe_folder.

@lasote lasote assigned memsharded and unassigned lasote Dec 3, 2019
@memsharded memsharded assigned lasote and unassigned memsharded Dec 3, 2019
@lasote lasote merged commit 0441503 into conan-io:develop Dec 3, 2019
2 checks passed
2 checks passed
continuous-integration/jenkins/pr-head This commit looks good
Details
license/cla Contributor License Agreement is signed.
Details
@memsharded memsharded deleted the memsharded:feature/set_version_current_folder branch Dec 3, 2019
memsharded added a commit to memsharded/conan that referenced this pull request Dec 3, 2019
* fix cwd issues in set_version() evaluation

* explicit self.recipe_folder
lasote added a commit that referenced this pull request Dec 3, 2019
* compatible_package build=missing

* move CONAN_EXPORTED and CONAN_IN_LOCAL_CACHE outside cmake_flags

* do not extract build_type and in_local_cache

* it is moved to the end of the command line

* Change help message for --build policy for create command (#6131)

* change build policy for create

* change to package name

* remove unused imports

* client.load() (#6140)

* Fixes #6044 Improve error with malformed settings yml (#6059)

* Fixes #6044 Improve error raised when 'settings.yml' cannot be parsed

* add tests for invalid settings YAML file

* Fix several issues with download command and revisions (#6138)

* fail if specifying rev with revs disabled

* fix reference with revisions

* add package revision to id

* add tests

* check revisions

* fix create without user channel

* add test without user channel

* minor changes

* skip depending on revisions

* add test with fake revision

* remove server from test

* test for older revision in server

* divide test

* Fix upload behaviour when using revisions for packages (#6143)

* wip

* fail if trying to upload different revision than in cache

* fail if rev specified without revs enabled

* add tests

* change line format

* fix python 27

* minor changes

* Fix SyntaxWarning in Python 3.8 (#6165)

* - generate ConfigVersion.cmake file (#6063)

Signed-off-by: SSE4 <tomskside@gmail.com>

* Fixes #5925 Make the first line from find program as the result (#6039)

* 3. extra separator in Windows

* Improve the error message when failed to connect to remote

* Improve the error message when failed to connect to remote

* Improve the error message when failed to connect to remote

* Revert unwanted changes

* Pick the first match which is the best match

* Pick the first match which is the best match

* make reversed explicit (better that [::-1])

* Fix output folder for conanworkspace.cmake when rebuilding dependencies (#6060)

* Added required = True to subparsers in order to print error message in Py2 and Py3.

* sync

* basic concurrent upload at reference level with futures

* revert changes

* add line

* Lock buggy urllib3 (#5808)

* app simplifying (#5806)

* Apply lockfile before updating downstream requires (#5771)

* apply graph_lock before looking for overrides

* first step: get rid of the warning

* cleaner if graph_lock is passed to the function

* only update requires upstream if no lockfile is applied

* fix tests

* Deprecation of CONAN_USERNAME and CONAN_CHANNEL: fix error message (#5756)

* if CONAN_USERNAME and CONAN_CHANNEL are deprecated, the error cannot recommend them

* update tests accordingly

* test client load() file method (#5815)

* no user/channel repr without _ (#5817)

* no user/channel repr without _

* minor fixes

* fix tests

* Remove py34 (#5820)

* fix upload package id (#5824)

* - update macOS, watchOS, tvOS, iOS version numbers (#5823)

* Refresh token client support.  (#5662)

* Refresh token client support. Missing tests. Missing migration

* public method

* WIP

* Refresh almost there

* Removed prints

* Try migrate

* Migration

* Add comment

* Refresh token flow following RFC recommentations

* Refresh ok

* review

* Remove traces

* Refactor capabilities

* Removed tmp file

* Review

* #5819 Show warning message for Python 3.4 (#5829)

* #5819 Show warning message for Python 3.4

- Add new warning message for python 3.4 which is no longer supported
- Added funcional tests to validate both python 3.4 and 2.x

Signed-off-by: Uilian Ries <uilianries@gmail.com>

* #5819 Fix broken tests

Signed-off-by: Uilian Ries <uilianries@gmail.com>

* Add cpp_info.name to cmake and pkg_config generators (#5598)

* Add cpp_info.name to cmake generators

* Fix unit tests to mimic real behavior

* cmake_paths test

* add test for cmake generator

* Add cmake_find_package test

* fix test in py3

* Applied cpp_info.name to pkg_config generator

* check different name in pkg_config

* sync with develop

* save conanworkspace.cmake relative to base folder

* add test

* siplify test

* add context manager to build folder

* change indent

* Publish 'artifacts.properties' using matrix params (#6014)

* draft to test

* add missing argument to functions

* rename 'put_headers' to 'artifact_properties'

* store matrix params as the preformatted string in the router

* fallback to empty string

* something going wrong: too many cchanges

* at least these are not needed

* typo

* it was only needed to remove the matrix_params for the bottle server

* soooo

* remove class name

* fix minor bug

* now with revisions too

* make docs with triple double quotes

* we need to modify the conan_server to accept matrix_params too

* fix tests

* add tests with artifacts.properties

* quote values of matrix params

* we can stack routes

* add tests with many different values

* remove changes not needed

* use server capability to choose the URL to build

* do not send headers if matrix_params capability

* do not add 'matrix_params' functionality to conan-server

* rename variable, 'file' is reserved

* remove change uneeded

* conan server does not support matrix params at all

* use the proper if/else

* Add res dir to the variables when using cmake_find_package. (issue 3722) (#6166)

* skip downloading call to to_file_bytes (#6142)

* remove syntax warning (#6174)

* fix cwd issues in set_version() evaluation (#6130)

* fix cwd issues in set_version() evaluation

* explicit self.recipe_folder
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.