SCons: fix release Python warnings #684
Conversation
|
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' |
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
That looks great! I would personally prefer an automated release process with CI, but in the meantime fixing the script would be awesome!
There was a problem hiding this comment.
An automated release process in CI and the script are not incompatible
There was a problem hiding this comment.
Looks fine. I was waiting for @fermino to accept. Will check back in a few days 🕚
And fix release version setter
|
Hey guys, I'm just back from a couple of vacation days and a (hopefully not bad) wrist accident. I saw tons of activity in the repo! I will try to catch up with all the emails but in the meantime: where is help/approvals needed? |
|
Regarding the PR: looks good to me! I don't want to merge it right now as I'm not sure if there's anything that you guys want merged before, but to me it's good to go. |
Ouch... take care!! |
3 participants
Closes #655