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

SCons: fix release Python warnings #684

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

vassilit
Copy link
Collaborator

Closes #655

@RayOei
Copy link
Collaborator

RayOei commented Mar 1, 2025

This would also improve PR #677

SConstruct Outdated
'sed -i "s/2\.0\.0/{v}/g" po/rmlint.pot',
'sed -i "s/^Version:\(\s*\)2\.0\.0/Version:\\1{v}/g" pkg/fedora/rmlint.spec'
r'sed -i "s/2\.0\.0/{v}/g" po/rmlint.pot',
r'sed -i "s/^Version:\(\s*\)2\.0\.0/Version:\\1{v}/g" pkg/fedora/rmlint.spec'
Copy link
Collaborator

Choose a reason for hiding this comment

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

The first line looks okay to me. I think the second one needs un-escaping the parentheses and the \1 reference. I'd like to have a test case for building in fedora, although I think this would be for a release with python 2...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nevermind, I totally misread it. For what I understand it replaces the version string in a translation and a packaging file. It is actually broken as the replaced string is 2.0.0 and the files are already in 2.8.0.

@sahib I'm guessing that was intended to be used locally when releasing tarballs in github?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@fermino
Indeed, I didn't look at the relevance of the call to sed, but just checked that sed was called with the right arguments and that it returned 0.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@fermino yes, those line are run only in release mode, when generating a tarball.
For the SPEC file, actually I would be wise to do Version: $(git describe --always --match "v*") or something along this line.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That looks great! I would personally prefer an automated release process with CI, but in the meantime fixing the script would be awesome!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

An automated release process in CI and the script are not incompatible

And fix release version setter
@vassilit vassilit requested a review from fermino March 2, 2025 14:59
@vassilit vassilit added this to the 2.10.3 To Be Determined milestone Mar 2, 2025
@vassilit vassilit removed the request for review from fermino March 3, 2025 19:35
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.

SyntaxWarning: invalid escape sequence
3 participants