Skip to content

Comments

[staging] cargo/hooks: allow hooks to be disabled#114716

Merged
FRidh merged 1 commit intoNixOS:stagingfrom
jonringer:fix-gnome-tour
Mar 8, 2021
Merged

[staging] cargo/hooks: allow hooks to be disabled#114716
FRidh merged 1 commit intoNixOS:stagingfrom
jonringer:fix-gnome-tour

Conversation

@jonringer
Copy link
Contributor

@jonringer jonringer commented Mar 1, 2021 •
edited
Loading

Motivation for this change

For gnome-tour we want meson to do the build, however, there was no way to conditionally disable the buildRustPackage hooks

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@ofborg ofborg bot added 6.topic: GNOME GNOME desktop environment and its underlying platform 6.topic: rust General-purpose programming language emphasizing performance, type safety, and concurrency. labels Mar 1, 2021
@ofborg ofborg bot requested review from dasj19, hedning, jtojnar and worldofpeace March 1, 2021 01:39
@ofborg ofborg bot added 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. labels Mar 1, 2021
@jonringer
Copy link
Contributor Author

jonringer commented Mar 1, 2021

I can also move this into staging, as gnome-tour seems to have limited use, and the rust changes cause significant rebuilds

@collares
Copy link
Member

collares commented Mar 1, 2021 •
edited
Loading

See also #114669 and #114673

@worldofpeace
Copy link
Contributor

worldofpeace commented Mar 1, 2021

gnome-tour has been fixed. It would only cause a conflict on master

@jonringer
Copy link
Contributor Author

jonringer commented Mar 1, 2021

@worldofpeace I can remove the gnome-tour commit, but i think the cargo hooks should remain

cole-h
cole-h reviewed Mar 1, 2021
View reviewed changes
danieldk
danieldk reviewed Mar 1, 2021
View reviewed changes
@danieldk
Copy link
Contributor

danieldk commented Mar 1, 2021 •
edited
Loading

Lookgs good to me. Could you also add the donts to the buildRustPackage documentation? I think I also agree with @cole-h 's nitpick about the naming.

@jonringer
Copy link
Contributor Author

jonringer commented Mar 1, 2021

/rebase-staging

@github-actions
Copy link
Contributor

github-actions bot commented Mar 1, 2021

Failed to rebase

@jonringer
Copy link
Contributor Author

jonringer commented Mar 1, 2021

/rebase staging

@jonringer
Copy link
Contributor Author

jonringer commented Mar 1, 2021

gnome-tour has been fixed. It would only cause a conflict on master

master gets merged into staging-next now through a bot

@ofborg ofborg bot removed the 6.topic: GNOME GNOME desktop environment and its underlying platform label Mar 1, 2021
@github-actions github-actions bot changed the base branch from staging-next to staging March 1, 2021 15:38
@github-actions
Copy link
Contributor

github-actions bot commented Mar 1, 2021

Rebased, please reopen the pull request to restart CI

@github-actions github-actions bot closed this Mar 1, 2021
@worldofpeace worldofpeace changed the title [staging-next] cargo/hooks: allow hooks to be disabled, gnome-tour: disable cargoBuildHook [staging] cargo/hooks: allow hooks to be disabled Mar 1, 2021
@jtojnar jtojnar mentioned this pull request Mar 3, 2021
Closed
@FRidh
Copy link
Member

FRidh commented Mar 8, 2021

This is why in my opinion we should not use generic phases such as buildPhase and configurePhase but instead have hooks provide their own phases. Unfortunately, we have to at least disable buildPhase and so on because stdenv.mkDerivation has them as default.

@FRidh FRidh merged commit 042adf0 into NixOS:staging Mar 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cole-h cole-h cole-h left review comments

@andir andir Awaiting requested review from andir

@zowoq zowoq Awaiting requested review from zowoq

@dasj19 dasj19 Awaiting requested review from dasj19

+3 more reviewers

@danieldk danieldk danieldk left review comments

@worldofpeace worldofpeace worldofpeace left review comments

@FRidh FRidh FRidh approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

6.topic: rust General-purpose programming language emphasizing performance, type safety, and concurrency. 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@jonringer @collares @worldofpeace @danieldk @FRidh @cole-h