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

alsa-ucm-conf: fix hardcoded FHS paths #371108

Merged
merged 2 commits into from
Jan 5, 2025
Merged

Conversation

drupol
Copy link
Contributor

@drupol drupol commented Jan 5, 2025

I recreated #371100 here, hopefully it won't ping all the people this time.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 25.05 Release Notes (or backporting 24.11 and 25.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@drupol drupol marked this pull request as ready for review January 5, 2025 12:18
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/dell-xps-13-9320-microphone-not-working/40932/8

@wolfgangwalther
Copy link
Contributor

There seem to be a few /sbin/ references, too: https://github.com/search?q=repo%3Aalsa-project%2Falsa-ucm-conf%20%2Fbin%2F&type=code

Not sure if they can/should be replaced, though.

@drupol
Copy link
Contributor Author

drupol commented Jan 5, 2025

Great, going to fix them too now.

@drupol drupol changed the title alsa-ucm-conf: fix hardcoded path to /bin/rm and /bin/mkdir alsa-ucm-conf: fix hardcoded FHS path Jan 5, 2025
@drupol drupol changed the title alsa-ucm-conf: fix hardcoded FHS path alsa-ucm-conf: fix hardcoded FHS paths Jan 5, 2025
@drupol drupol force-pushed the push-ytnprzvyuluv-1 branch from 9613a91 to c18ebb2 Compare January 5, 2025 14:06
@nix-owners nix-owners bot requested a review from roastiek January 5, 2025 14:14
@hraban
Copy link
Member

hraban commented Jan 5, 2025

@drupol re #371100 (comment) What happened there is that your PR contained commits from master that weren't in staging yet, and when you changed the merge base from master to staging a bot detects those commits and asks for a review of everyone who is a relevant owner for anything touched by any of those commits. It's a bit of a bug in the CI automation of nixpkgs if you ask me but it only goes wrong once every ~3mo, so for now people just seem to live with it :)

The nixpkgs way to change base is to first rebase your new commits onto $(git merge-base upstream/{master,staging}), force push that, then change the PR target in github. Finally, if you wish, rebase your changes back on top of the latest commit of master or staging, if that makes you happy.

@wolfgangwalther
Copy link
Contributor

It's a bit of a bug in the CI automation of nixpkgs if you ask me but it only goes wrong once every ~3mo, so for now people just seem to live with it :)

It's not supposed to go wrong anymore. code-owners-v2 has fixed this for code-owner review requests and if it wasn't for the recent change from ofborg to GA for maintainer requests, it wouldn't happen for those cases either.

So yes, it was a bug, but a very recent one, introduced yesterday, fixed today.

@hraban
Copy link
Member

hraban commented Jan 5, 2025

It's a bit of a bug in the CI automation of nixpkgs if you ask me but it only goes wrong once every ~3mo, so for now people just seem to live with it :)

It's not supposed to go wrong anymore. code-owners-v2 has fixed this for code-owner review requests and if it wasn't for the recent change from ofborg to GA for maintainer requests, it wouldn't happen for those cases either.

So yes, it was a bug, but a very recent one, introduced yesterday, fixed today.

Good to hear, although I'm surprised to hear "introduced yesterday" part because I've run into this problem myself, and been pinged when others did. Am I talking about something else? Pretty sure "changing the target branch of a github PR" has been a dangerous thing to do since I joined >2y ago.

@drupol
Copy link
Contributor Author

drupol commented Jan 5, 2025

Feel free to also review this PR 😆 !

@FliegendeWurst
Copy link
Member

Pretty sure "changing the target branch of a github PR" has been a dangerous thing to do since I joined >2y ago.

It is fairly safe since #347610. Worst case is some excessive labeling.

@drupol drupol merged commit f58a39c into NixOS:staging Jan 5, 2025
23 of 27 checks passed
@drupol drupol deleted the push-ytnprzvyuluv-1 branch January 5, 2025 21:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants