Skip to content

hare: unstable-2023-04-23 -> unstable-2023-10-22; harec: unstable-2023-04-23 -> unstable-2023-10-23#266456

Merged
pbsds merged 1 commit intoNixOS:masterfrom
onemoresuza:hare
Nov 24, 2023
Merged

hare: unstable-2023-04-23 -> unstable-2023-10-22; harec: unstable-2023-04-23 -> unstable-2023-10-23#266456
pbsds merged 1 commit intoNixOS:masterfrom
onemoresuza:hare

Conversation

@onemoresuza
Copy link
Contributor

@onemoresuza onemoresuza commented Nov 9, 2023

Description of changes

Update hare and harec to the refs on Alpine and do the following refactoring:

hare:

  1. Replace NIX_BUILD_TOP usage with mktemp
  2. Enable parallel building
  3. Get rid of config-template.mk, by using makeFlagsArray instead
  4. Move wrapProgram call to postFixup
  5. Set the LOCALVER environment variable to nix, so that the hare version command outputs dev-nix1
  6. Set meta attribute mainProgram
  7. Make the package accessible through all-packages.nix instead of
    aliases.nix
  8. Move package path from pkgs/development/compilers/hare/hare to
    pkgs/development/compilers/hare, since the scope is now made at
    pkgs/top-level/hare-packages.nix

harec:

  1. Remove hardeningDisable = [ "fortify" ];, since both harec and hare
    compile normally with it enabled
  2. Enable parallel building
  3. Set meta attribute mainProgram
  4. Make the package accessible through all-packages.nix instead of
    aliases.nix
  5. Move package path from pkgs/development/compilers/hare/harec to
    pkgs/development/compilers/harec, since the scope is now made at
    pkgs/top-level/hare-packages.nix

harePackages:

  1. Move hare packages scope from
    pkgs/development/compilers/hare/default.nix to
    pkgs/top-level/hare-packages.nix
  2. Remove hare and harec from aliases.nix, moving them to
    all-packages.nix
  3. Change the callPackage argument in all-packages.nix from
    ../development/compilers/hare to ./hare-packages.nix

Co-authored-by: starzation nixpkgs@starzation.net

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/)
  • 23.11 Release Notes (or backporting 23.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.

@onemoresuza
Copy link
Contributor Author

onemoresuza commented Nov 9, 2023

Output of nixpkgs-review rev HEAD:

Result of nixpkgs-review run on x86_64-linux 1

1 package marked as broken and skipped:
  • himitsu-firefox
1 package failed to build:
  • himitsu
2 packages built:
  • harePackages.hare
  • harePackages.harec

The himitsu package fails to build because of some changes to Hare's stdlib; when updated to 0.4, It will work again.

@ofborg ofborg bot added 11.by: package-maintainer This PR was created by a maintainer of all the package it changes. 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Nov 9, 2023
@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/prs-ready-for-review/3032/2899

Copy link
Contributor

@majiru majiru left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comments around the temporary changes are nice and the refactoring seems like an improvement to me.

@delroth delroth added the 12.approvals: 1 This PR was reviewed and approved by one person. label Nov 9, 2023
@pbsds
Copy link
Member

pbsds commented Nov 9, 2023

Due to the himitsu breakage this is likely blocked until after the 23.11 branch-of, since himitsu 0.4 looks to be a breaking change. Feel free to ping me after the branch-of 😃

@onemoresuza
Copy link
Contributor Author

Due to the himitsu breakage this is likely blocked until after the 23.11 branch-of, since himitsu 0.4 looks to be a breaking change. Feel free to ping me after the branch-of 😃

No worries. Should I do something to this PR to indicate that it is waiting for the 23.11 release? Perhaps renaming the title or adding a label?

@pbsds
Copy link
Member

pbsds commented Nov 10, 2023

I don't think we have a convention for such. I've been toying with the idea of proposing a post-ZHF merge queue, and a wait-for-maintainer-feedback-but-a-committer-wants-to-merge-it merge queue.

@onemoresuza
Copy link
Contributor Author

onemoresuza commented Nov 21, 2023

@majiru, @pbsds, I was thinking about the changes I've made to qbe on both hare and harec.

As of now:

  1. if the user overrides the qbe argument of harec with override, any change made to its src or version would not matter, since they are locally overwritten.
  2. Moreover, the change made to hare will cause breakage to any code that needs the qbe argument on it, since it is now removed.

Are those acceptable changes?

Note: Do feel free to ignore this comment, if this is considered a misused ping.

@starzation
Copy link
Contributor

starzation commented Nov 22, 2023

@onemoresuza Maybe you should update this to latest version (sorry for ping if it anoy you)

@onemoresuza
Copy link
Contributor Author

No worries, @starzation.

I'm planning to do this, but firstly this PR must be merged: the most recent versions of Hare need the leap-seconds.list file for both its test suite and its timekeeping needs.

Copy link
Contributor

@starzation starzation left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

homepage should be use https, other than these is good

@pbsds
Copy link
Member

pbsds commented Nov 22, 2023

Are those acceptable changes?

I think this is fine. It's still overrideable, it just requires a bit more effort. And there is always the possibility to change this later

@ofborg build hare hare.passthru.tests harec harec.passhtru.tests

@onemoresuza
Copy link
Contributor Author

@starzation, I think this PR would be a good place for the creation of the pkgs/top-level/hare-packages.nix scope that you created in your PR for hare-json.

Would you be okay if I brought that change to this PR and add you as a co-author of this commit? If so, could you please give the email to be appended to your username on the git trailer, i. e., either a real email or the github no-reply one1?

Footnotes

  1. Which can be found here https://github.com/settings/emails under the Keep my email addresses private checkbox.

@starzation
Copy link
Contributor

@starzation, I think this PR would be a good place for the creation of the pkgs/top-level/hare-packages.nix scope that you created in your PR for hare-json.

Would you be okay if I brought that change to this PR and add you as a co-author of this commit? If so, could you please give the email to be appended to your username on the git trailer, i. e., either a real email or the github no-reply one1?

Footnotes

1. Which can be found here https://github.com/settings/emails under the `Keep my email addresses private` checkbox. [↩](#user-content-fnref-1-d983b488c108cc4973a256603d5f9d47)

Sure, I'm okay with that. You can use this email address nixpkgs@starzation.net

Copy link
Member

@SuperSandro2000 SuperSandro2000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

otherwise LGTM

Comment on lines 63 to 69
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those don't work in makeFlags?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the exception of HARECACHE and BINOUT, they would. I've put them all together in makeFlagsArray so that the make flags are not split between makeFlags and makeFlagsArray.

But if it the ones that work on makeFlags should necessarily be in it, I can move them there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A note: github's outdated warning is because the file path has changed from pkgs/development/compilers/hare/hare/default.nix to pkgs/development/compilers/hare/default.nix. The makeFlagsArray has not changed.

…3-04-23 -> unstable-2023-10-23

And also the following refactoring:

hare:

1. Replace `NIX_BUILD_TOP` usage with `mktemp`
2. Enable parallel building
3. Get rid of `config-template.mk`, by using `makeFlagsArray` instead
4. Move `wrapProgram` call to `postFixup`
5. Set the `LOCALVER` environment variable to `nix`, so that the `hare
   version` command outputs `dev-nix`[1]
6. Set `meta` attribute `mainProgram`
7. Make the package accessible through `all-packages.nix` instead of
   `aliases.nix`
8. Move package path from `pkgs/development/compilers/hare/hare` to
   `pkgs/development/compilers/hare`, since the scope is now made at
   `pkgs/top-level/hare-packages.nix`

harec:

1. Remove `hardeningDisable = [ "fortify" ];`, since both harec and hare
   compile normally with it enabled
2. Enable parallel building
3. Set `meta` attribute `mainProgram`
4. Make the package accessible through `all-packages.nix` instead of
   `aliases.nix`
5. Move package path from `pkgs/development/compilers/hare/harec` to
   `pkgs/development/compilers/harec`, since the scope is now made at
   `pkgs/top-level/hare-packages.nix`

harePackages:

1. Move hare packages scope from
   `pkgs/development/compilers/hare/default.nix` to
   `pkgs/top-level/hare-packages.nix`
2. Remove `hare` and `harec` from `aliases.nix`, moving them to
   `all-packages.nix`
3. Change the `callPackage` argument in `all-packages.nix` from
   `../development/compilers/hare` to `./hare-packages.nix`

[1]: https://git.sr.ht/~sircmpwn/hare/tree/1048620a7a25134db370bf24736efff1ffcb2483/item/scripts/version#L2

Co-authored-by: starzation <nixpkgs@starzation.net>
@onemoresuza
Copy link
Contributor Author

onemoresuza commented Nov 23, 2023

The ofborg error was on the ofborg-eval-nixpkgs-manual. I don't think it's related to the pushed changes since the error occurs while evaluating the attribute 'buildPhase' of the derivation 'nixpkgs-manual', which I believe is not affected by this PR.

Any thoughts on this?

It was just a coincidence. A new evaluation succeeded.

@ofborg ofborg bot added the 8.has: package (new) This PR adds a new package label Nov 24, 2023
@pbsds
Copy link
Member

pbsds commented Nov 24, 2023

Result of nixpkgs-review pr 266456 run on x86_64-linux 1

1 package marked as broken and skipped:
  • himitsu-firefox
1 package failed to build:
  • himitsu
2 packages built:
  • hare (harePackages.hare)
  • harec (harePackages.harec)

Himitsu to be fixed in #267060, LGTM, should not be backported. Thanks!

@pbsds pbsds merged commit 1244907 into NixOS:master Nov 24, 2023
@onemoresuza onemoresuza deleted the hare branch November 24, 2023 21:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

8.has: package (new) This PR adds a new package 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 11.by: package-maintainer This PR was created by a maintainer of all the package it changes. 12.approvals: 1 This PR was reviewed and approved by one person.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants