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

Use 'revision' as the SVN's 'peg_revision' (broken for an edge case) #5029

Merged
merged 5 commits into from May 21, 2019

Conversation

Projects
None yet
6 participants
@jgsogo
Copy link
Member

commented Apr 25, 2019

Changelog: Fix: Use revision as the SVN's peg_revision (broken for an edge case)
Docs: omit

closes #5017

@tags: svn, slow

@ghost ghost assigned jgsogo Apr 25, 2019

@ghost ghost added the stage: review label Apr 25, 2019

jgsogo added some commits Apr 25, 2019

@jgsogo

This comment has been minimized.

Copy link
Member Author

commented May 3, 2019

Motivated by #5017 we are thinking about changing get_last_changed_revision by get_revision to retrieve the peg_revision for the URL. It is required for a use-case where it is being checked out a subtree of a tag to create the package (more details in the issue, and the test in this PR).

IMO, although it could lead to a different peg_revision it would still be valid, and we will solve this use case. I'm pinging our SVN users @climblinne, @Aalmann, just in case they can point out some unexpected behavior due to this change. Thanks!

@jgsogo jgsogo marked this pull request as ready for review May 3, 2019

@climblinne
Copy link
Contributor

left a comment

Looks good to me.

@lasote

This comment has been minimized.

Copy link
Contributor

commented May 9, 2019

@Aalmann could you take a look? Thanks!

@Aalmann

This comment has been minimized.

Copy link

commented May 10, 2019

Looks good to me.
@ahauan4 can you test it with one of your repos? #5017 describes a similar layout to yours.

@ahauan4

This comment has been minimized.

Copy link

commented May 10, 2019

@Aalmann We had not a problem so far, because we check out the trunk/branches/tag directory directly on Jenkins.
Our setup is: repo_root/category_name/module_name/<trunk|branches|tags>/conanfile.py

One disadvantage of using get_revision instead of get_last_changed_revision could be, that if we build the same thing twice after the revision counter increased (because of checkins in a different module), we would get different revisions published in the final package. But as long as this won't affect the hash, it would be ok from my side.

@lasote lasote added this to the 1.16 milestone May 13, 2019

@lasote

This comment has been minimized.

Copy link
Contributor

commented May 13, 2019

@Aalmann @ahauan4 Are you using the package revisions mechanism? In that case, if you export the recipe again you will get new package revision because of the captured URL with change with the new (svn) revision counter. But the package_id won't change in any case.
Are you ok then?

@jgsogo

This comment has been minimized.

Copy link
Member Author

commented May 13, 2019

The change from get_last_changed_revision to get_revision is done only for the peg_revision but not for the revision to checkout, so in your case, @ahauan4, the problem was there and will still be.

About the hash: each time the recipe is exported (if there is any "auto" in the SCM dictionary) those "auto" will be resolved and substituted. If there are new commits, then the peg_revision and the revision will change (the second one for sure, and the first one since this PR) and the hash of the recipe will change (but it was changing before too).

@ahauan4

This comment has been minimized.

Copy link

commented May 13, 2019

@lasote @jgsogo We don't use the package revisions feature.
I meant package_id instead of hash. Sorry for beeing unclear.
So as long as the package_id won't change, we should be ok.

@Aalmann

This comment has been minimized.

Copy link

commented May 14, 2019

@lasote and @jgsogo
We are not using the package revisions at the moment. So everything is fine. Go for it.

@jgsogo

This comment has been minimized.

Copy link
Member Author

commented May 14, 2019

Thanks all for your feedback! 😸

@keithrob91, if it is not blocking you, we will release it with 1.16, is it ok?

@keithrob91

This comment has been minimized.

Copy link

commented May 15, 2019

That is fine. Thank you.

@lasote lasote merged commit 67fb2df into conan-io:develop May 21, 2019

2 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
license/cla Contributor License Agreement is signed.
Details

@jgsogo jgsogo deleted the jgsogo:issue/5017-svn-tag branch May 21, 2019

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.