-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
niri: update to 25.01 #53946
base: master
Are you sure you want to change the base?
niri: update to 25.01 #53946
Conversation
tested and it seems to work fine. |
Can confirm that it works just fine! |
Another confirmation that all is good... on a musl installation here. |
Works fine for me! |
srcpkgs/niri/template
Outdated
checksum=d8854830436a87215b0bc6a60b6d43f350d927a03a2798c75f0fbda228bac8d3 | ||
checksum=86b89bcfc3fc6a8ed81f9e3f0ac7a29bd30267515efb2c19e1e0bc2ccd67b649 | ||
|
||
export NIRI_BUILD_COMMIT="e35c630" |
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.
would be better to use something like v$version
, as
- hardcoding things like that is bad and will be forgotten at some point
- we build from the tag, which is a reference to the commit
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.
This variable is in accordance with Niri's own packaging guidelines, it is on top of the reported version number, i.e. niri 25.01 (e35c630)
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.
upstreams do stuff like this all the time. if they aren't leaving the commit in their release tarballs somehow (like a file), we shouldn't need to hard-code it. we aren't building some random commit, we're building the release, so the commit information is redundant.
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.
Understood and agreed that it is redundant, however if it is expected by upstream, I'm not sure I really think it is worth it to break that expectation downstream is all (i.e. for bug reports and such)
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.
If upstream wants this information to be accurate, they should add it to their release artifacts. they already generate them to vendor cargo dependencies.
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.
Fair point, I might poke upstream to add that, but then tbh I think it is best to just not add any environment variable as @trolljoe did now, since it will report (unknown commit)
by default, which is more meaningful here
srcpkgs/niri/template
Outdated
checksum=86b89bcfc3fc6a8ed81f9e3f0ac7a29bd30267515efb2c19e1e0bc2ccd67b649 | ||
|
||
export NIRI_BUILD_COMMIT="e35c630" | ||
export XDG_RUNTIME_DIR="$(mktemp -d)" |
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.
maybe:
pre_check() {
export XDG_RUNTIME_DIR="$mktemp -d "$wrksrc/runtime.XXXXXX")"
}
this will keep things contained inside the build directory and make it only used for tests
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.
is the syntax correct? wouldn't i use this instead?
pre_check() {
export XDG_RUNTIME_DIR=$(mktemp -d "$wrksrc/runtime.XXXXXX")
}
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.
Methinks it is a typo, but mind the additional quotes:
pre_check() {
export XDG_RUNTIME_DIR="$(mktemp -d "$wrksrc/runtime.XXXXXX")"
}
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.
yes mine was missing a (
i will update the template including that, my bad |
Np, there may be legitimate reasons for it actually since I am not 100% sure what it does (but Works For Me™ without |
@trolljoe it wasn't enabled in the former version, if I'm right? However, I didn't have recognised any drawbacks even without |
Testing the changes
this goes by the new packaging niri page (https://github.com/YaLTeR/niri/wiki/Packaging-niri), if setting XDG_RUNTIME_DIR to
mktemp
isn't enough i'll skip the tests.