-
Notifications
You must be signed in to change notification settings - Fork 228
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
Build: Debian/Ubuntu: Avoid incorrect -dirty version suffix #2802
Build: Debian/Ubuntu: Avoid incorrect -dirty version suffix #2802
Conversation
Did that happen starting from the introduction of the flag? |
7d30af6
to
43cc744
Compare
PR logs look fine: https://github.com/jamulussoftware/jamulus/runs/7982212320?check_suite_focus=true#step:11:80 |
# dh_update_autotools_config replaces libs/opus/config.{sub,guess}. | ||
# This is unnecessary as we don't build opus via autotools at all | ||
# (we use qmake). In addition, this causes our -dev version generation | ||
# logic to mark Debian builds as -dirty by default. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd add a link to this PR here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, why do that here? We don't add Github Issue/PR links for other code parts either. If you think that something is missing in the comment, I'm happy to add it. If someone wants to track that back to Github, this should be possible using git blame
and git log
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You describe an issue which is fixed. "In addition ... by default." Therefore I thought it would make sense to link this PR for further information.
You could also change the grammar: ... this causes --> This would have caused...
Which would – at least for me – make the reference to this PR less "needed" in the code itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I've added the would part in the hope that it makes it clearer. The point of the comment is to state that there are two reasons:
- First, the config.guess replacement behavior is unnecessary, as the file is not used at all.
- Second (therefore "in addition"), it causes the
-dirty
behavior.
The debian build scripts will update config.{guess,sub} and will therefore cause the Jamulus.pro dev version detection to mark the build as dirty. We circumvent that by overriding the responsible debian helper call. Fixes: jamulussoftware#2800
43cc744
to
b6ef414
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I'm happy now
Short description of changes
The debian build scripts will update config.{guess,sub} and will therefore cause the Jamulus.pro dev version detection to mark the build as dirty. We circumvent that by overriding the responsible debian helper call.
CHANGELOG: Debian/Ubuntu: Fixed displayed version for non-release builds to remove incorrect -dirty suffixes.
Context: Fixes an issue?
Fixes: #2800
Does this change need documentation? What needs to be documented and how?
No.
Status of this Pull Request
Ready.
What is missing until this pull request can be merged?
Reviews.
Checklist