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

Evaluate always SCM attribute before exporting the recipe #4088

Merged
merged 5 commits into from Dec 18, 2018

Conversation

@jgsogo
Copy link
Member

commented Dec 7, 2018

Changelog: Fix: Evaluate always SCM attribute before exporting the recipe
Docs: conan-io/docs#981

  • closes #3831 (needs confirmation)

--- Edited by @lasote ---


If the SCM attributes rely on data/status of the working copy we need to evaluate the SCM attributes there. It happens with the auto keywords, but it can happen with many other functions:

  • every function that needs to run a CVS (this is the test I have added)
  • a function that makes use of imported python from a file that belongs to the repo
  • ...

All those data/status won't be available in the exported folder of the cache.

@ghost ghost assigned jgsogo Dec 7, 2018

@ghost ghost added the stage: review label Dec 7, 2018

@jgsogo jgsogo requested a review from memsharded Dec 7, 2018

@jgsogo jgsogo added this to the 1.11 milestone Dec 7, 2018

@jgsogo jgsogo added the type: bug label Dec 7, 2018

@danimtb
Copy link
Member

left a comment

I think I would need a further explanation about these changes

Show resolved Hide resolved conans/test/functional/scm_test.py Outdated

@jgsogo jgsogo referenced this pull request Dec 12, 2018

Closed

[SCM][SVN] Support working in a mono-repo checkout #3786

3 of 3 tasks complete
@lasote

lasote approved these changes Dec 18, 2018

Copy link
Contributor

left a comment

I would like to see if it works with python_requires (or clarify that it doesn't). Also, we have to document this with an example in the docs.

@ghost ghost assigned memsharded Dec 18, 2018

@memsharded memsharded merged commit 3abd192 into conan-io:develop Dec 18, 2018

2 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
license/cla Contributor License Agreement is signed.
Details
@jgsogo
Copy link
Member Author

left a comment

What do you think about these comments, @memsharded?

def get_commit():
here = os.path.dirname(__file__)
here = os.getcwd()

This comment has been minimized.

Copy link
@jgsogo

jgsogo Dec 18, 2018

Author Member

It is not exactly the same... as a test (someone can look at it as an example) I prefer the previous behavior.

This comment has been minimized.

Copy link
@memsharded

memsharded Dec 18, 2018

Contributor

With __file__ it fails. The file of the python require is located in a different place than the repo

content = load(exported_conanfile)
self.assertIn(commit, content)
# Check that everything works and is usable

This comment has been minimized.

Copy link
@jgsogo

jgsogo Dec 18, 2018

Author Member

🤔 I will remove these lines because they are related to python_requires machinery, not to the replacement of the SCM information. I know it is the reason why my previous test was wrong, but now that it is ok these lines are only adding more time to the test suite. If not, can they be conditionally included to the slow attribute tag?

@ghost ghost removed the stage: review label Dec 18, 2018

@jgsogo

This comment has been minimized.

Copy link
Member Author

commented Dec 18, 2018

@lasote, do you think we need to write an example using this? I don't think so, it is expected behavior, just the combination of py_req and the early resolution of SCM

@jgsogo jgsogo deleted the jgsogo:issue/3831 branch Dec 18, 2018

@jgsogo

This comment has been minimized.

Copy link
Member Author

commented Dec 19, 2018

Updated with a link to the docs

grisumbras pushed a commit to grisumbras/conan that referenced this pull request Dec 27, 2018

Merge pull request conan-io#4088 from jgsogo/issue/3831
Evaluate always SCM attribute before exporting the recipe
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.