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.

Sign up for GitHub

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

Jump to bottom

sources: get svn revision with --show-item flag #1648

Merged
merged 1 commit into from Nov 8, 2017
Merged

sources: get svn revision with --show-item flag #1648

merged 1 commit into from Nov 8, 2017

Conversation

clobrano
Copy link
Contributor

@clobrano clobrano commented Nov 1, 2017

Currently, svn last changed revision is extracted from svn info
text output. The same information can be retrieved directly from
the command

svn info --show-item last-changed-revision

without the need to parse text.

Adding also the flag --no-newline to avoid the need to strip
newline character from the output.

Since there is not need to parse text anymore, this also remove
the dependency from the current locale and then
fixes https://bugs.launchpad.net/snapcraft/+bug/1724674

  • Have you followed the guidelines for contributing?
  • Have you signed the CLA?
  • If this is a bugfix. Have you checked that there is a bug report open for the issue you are trying to fix on bug reports?
  • If this is a new feature. Have you discussed the design on the forum?
  • Have you successfully run ./runtests.sh static?
  • Have you successfully run ./runtests.sh unit?

@clobrano
sources: get svn revision with --show-item flag
d9f8435 
Currently, svn last changed revision is extracted from `svn info`
text output. The same information can be retrieved directly from
the command

`svn info --show-item last-changed-revision`

without the need to parse text.

Adding also the flag `--no-newline` to avoid the need to strip
newline character from the output.

Since there is not need to parse text anymore, this also remove
the dependency from the current locale and then
fixes https://bugs.launchpad.net/snapcraft/+bug/1724674
come-maiz
come-maiz approved these changes Nov 1, 2017
View reviewed changes
Copy link
Contributor

@come-maiz come-maiz left a comment

Choose a reason for hiding this comment

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

So much better ^_^
Thanks!

@clobrano
Copy link
Contributor Author

clobrano commented Nov 1, 2017

😄

kalikiana
kalikiana reviewed Nov 3, 2017
View reviewed changes
Copy link
Contributor

@kalikiana kalikiana left a comment

Choose a reason for hiding this comment

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

Nice simplification!

Can we by any chance unit test this? The fact that the test pass unchanged concerns me slightly...

facundobatista
facundobatista reviewed Nov 3, 2017
View reviewed changes
snapcraft/internal/sources/_subversion.py
commit = subprocess.check_output(
['svn', 'info',
'--show-item', 'last-changed-revision',
'--no-newline',
Copy link
Contributor

Choose a reason for hiding this comment

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

MUCH better code, as we don't parse magical tokens! THANKS!

Why is the --no-newline parameter needed?

Maybe it's better (as simpler) without it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@facundobatista since we are already using str.strip, --no-newline should not be necessary, however I decided to keep it to make it more explicit.

@sergiusens
Copy link
Collaborator

sergiusens commented Nov 3, 2017 via email

@sergiusens sergiusens mentioned this pull request Nov 3, 2017
Closed
6 tasks
kalikiana
kalikiana approved these changes Nov 3, 2017
View reviewed changes
Copy link
Contributor

@kalikiana kalikiana left a comment

Choose a reason for hiding this comment

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

How very true... even though they are unit tests. I keep getting this mixed up.

In that case nevermind my comment. This is good.

kyrofa
kyrofa approved these changes Nov 3, 2017
View reviewed changes
Copy link
Contributor

@kyrofa kyrofa left a comment

Choose a reason for hiding this comment

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

This is excellent indeed, thank you @clobrano 👍

@clobrano
Copy link
Contributor Author

clobrano commented Nov 3, 2017

@ElOpio found the solution, I just implemented it, but thank you :)

@sergiusens
Copy link
Collaborator

@kalikiana can you make sure this works manually as well?

@kalikiana
Copy link
Contributor

kalikiana commented Nov 7, 2017 •
edited

Verifying the tests pass:
LANGUAGE=es python3 -m unittest run snapcraft.tests.sources.test_subversion.SubversionDetailsTestCase.test_svn_details_commit -f

I couldn't actually find a real world case that is affected. I thought at first LANGUAGE=en SNAPCRAFT_BUILD_INFO=1 snapcraft in a project using svn would have hit the problem but it works perfectly... @clobrano Did you hit the issue building an actual snap as well?

@clobrano
Copy link
Contributor Author

clobrano commented Nov 7, 2017

Not with a real snap, but running the test in a VM with es_AR locale

@sergiusens sergiusens added this to the 2.35 milestone Nov 8, 2017
@sergiusens sergiusens added the Bug Actual bad behavior that don't fall into maintenance or documentation label Nov 8, 2017
@sergiusens sergiusens merged commit 46fa607 into canonical:master Nov 8, 2017
matiasb pushed a commit to matiasb/snapcraft that referenced this pull request Nov 24, 2017
@clobrano @matiasb
sources: get svn revision with --show-item flag (canonical#1648)
40e67a4 
Currently, svn last changed revision is extracted from `svn info`
text output. The same information can be retrieved directly from
the command

`svn info --show-item last-changed-revision`

without the need to parse text.

Adding also the flag `--no-newline` to avoid the need to strip
newline character from the output.

Since there is not need to parse text anymore, this also remove
the dependency from the current locale.

LP: #1724674
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@facundobatista facundobatista facundobatista left review comments

@come-maiz come-maiz come-maiz approved these changes

@kyrofa kyrofa kyrofa approved these changes

@kalikiana kalikiana kalikiana approved these changes

Assignees
No one assigned
Labels
Bug Actual bad behavior that don't fall into maintenance or documentation
Projects
17.10
Awaiting triage
Milestone
2.35
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@clobrano @sergiusens @kalikiana @facundobatista @come-maiz @kyrofa