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

Opt-in to store SCM evaluated data into 'conandata.yml' file #6334

Merged
merged 7 commits into from Jan 15, 2020

Conversation

jgsogo
Copy link
Member

@jgsogo jgsogo commented Jan 10, 2020

Changelog: Feature: Add opt-in scm_to_conandata for the SCM feature: Conan will store the data from the SCM attribute in the conandata.yml file (except the fields username and password).
Docs: conan-io/docs#1522

Add an opt-in to store the fields evaluated from the scm attribute into the conandata.yml file (all fields except username and password). This way the recipe itself, the conanfile.py is not modified by Conan. Note.- This could be the default behavior for Conan 2.0 and remove the previous implementation.

closes #6275

  • Fields in the conandata.yml file (to be defined):

    .conan:
        scm:
            url: ....
            revision: ...
  • Config variable name: general.scm_to_conandata

@jgsogo jgsogo added this to the 1.22 milestone Jan 10, 2020
@jgsogo jgsogo self-assigned this Jan 10, 2020
conanfile.conan_data = conan_data
if conan_data and '.conan' in conan_data:
scm_data = conan_data['.conan'].get('scm_data')
if scm_data:
Copy link
Member Author

@jgsogo jgsogo Jan 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should ALWAYS be read from the conandata.yml because, regardless of the configuration in the user client consuming the recipe, if the packager has the opt-in activated and it is exporting the information in the conandata.yml file, the consumer has to read and use it.

@jgsogo
Copy link
Member Author

@jgsogo jgsogo commented Jan 10, 2020

Can you provide some early feedback/review about the implementation, @danimtb? I feel like this is all that we need... do you miss anything, any test? Thanks! 🙌

Copy link
Member

@danimtb danimtb left a comment

Looking good. I was thinking about additional tests using a recipe with scm as a consumer and other commands that might use the conandata outside the cache like conan inspect but I am not sure if those cases are involved

conans/client/cmd/export.py Show resolved Hide resolved
@jgsogo
Copy link
Member Author

@jgsogo jgsogo commented Jan 13, 2020

I could duplicate all the existing tests related to SCM, but I don't want to add more and more tests if they are not needed:

  • auto fields are only evaluated when exporting the recipe to the cache (nothing is done for local commands). At that moment I'm storing the data in the conandata.yml instead of modifying the conanfile.py
  • everytime the conanfile.py is read, Conan also parses the conandata.yml file and the SCM value is updated (it takes the same value as if the conanfile.py had been modified)

The only problem could be a conandata.yml file in the local folder containing the field .conan/scm_data, with this PR Conan will read it and update the SCM values... but that would be an error during export becuase this field is reserved to Conan (https://github.com/conan-io/conan/pull/6334/files#diff-19f7753b110692cddd8836812d4d8393R109)

@jgsogo jgsogo marked this pull request as ready for review Jan 13, 2020
if conan_data and '.conan' in conan_data:
scm_data = conan_data['.conan'].get('scm_data')
if scm_data:
conanfile.scm.update(scm_data)
Copy link
Member Author

@jgsogo jgsogo Jan 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

conanfile.scm HAS TO exist, otherwise, the conandata.yml wouldn't have a .coann/scm_data field

@jgsogo jgsogo requested a review from danimtb Jan 13, 2020
Copy link
Member

@memsharded memsharded left a comment

I think this is looking very good and no risk.

I am fine with .conan "protected" var, but would use scm for the field.
Also good general.scm_to_conandata to me.

Please have a look at the minor comments.

@jgsogo jgsogo assigned danimtb and unassigned jgsogo Jan 14, 2020
@jgsogo jgsogo merged commit 423b8b5 into conan-io:develop Jan 15, 2020
2 checks passed
@jgsogo jgsogo deleted the feature/scm-conandata branch Jan 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants