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.

Sign up for GitHub

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

Jump to bottom

nim: 0.19.4 -> 0.20.0 #62845

Merged
merged 2 commits into from Jun 26, 2019
Merged

nim: 0.19.4 -> 0.20.0 #62845

merged 2 commits into from Jun 26, 2019

Conversation

royneary
Copy link
Contributor

@royneary royneary commented Jun 8, 2019 •
edited by veprbl

Motivation for this change

Update to the latest release of Nim.

What I did:

  • makeWrapper not needed anymore
  • link sqlite (needed for the the stdlib sqlite module)
  • update nodejs to 11.x
  • use default phase order (fixes linker errors during test phase)
  • substitution in ./test/async/tioselectors.nim not needed anymore
  • two more tests need to be disabled
  • LD=$CC workaround not needed anymore
  • update mosdepth for it to compile with nim 0.20.0

nrpl compiles with the new version.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nix-review --run "nix-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)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@royneary royneary mentioned this pull request Jun 8, 2019
Closed
10 tasks
mmahut
mmahut approved these changes Jun 9, 2019
View reviewed changes
Copy link
Member

@mmahut mmahut left a comment

Choose a reason for hiding this comment

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

  • reviewed the diff and commit messages
  • made sure ofBorg evals
  • run nix-review without any failures
  • run and tested the binaries

@veprbl
Copy link
Member

veprbl commented Jun 9, 2019

@GrahamcOfBorg build nim mosdepth nrpl

@veprbl
Copy link
Member

veprbl commented Jun 9, 2019

cc @ehmry @peterhoeg

@royneary
Copy link
Contributor Author

Some tests lack support for aarch64. I disabled them for the architecture.

@peterhoeg
Copy link
Member

Nice cleanups as well! Could you please run the relevant pre and post hooks from the various phases as well?

Also, are you sure that tzdata isn't needed a proper buildInput?

@royneary
Copy link
Contributor Author

royneary commented Jun 10, 2019 •
edited

@peterhoeg: I'm triggering the pre and post hooks now (and don't override patchPhase anymore).

I think tzdata was only added as a dependency to fix the path /usr/share/zoneinfo in the test case ./tests/stdlib/ttimes.nim. So it's only needed during the checkPhase and not as a buildInput.

@veprbl
Copy link
Member

veprbl commented Jun 10, 2019

I think tzdata was only added as a dependency to fix the path /usr/share/zoneinfo in the test case ./tests/stdlib/ttimes.nim. So it's only needed during the checkPhase and not as a buildInput.

You mean this?

nixpkgs/pkgs/development/compilers/nim/default.nix

Line 69 in 567a05f

substituteInPlace ./tests/stdlib/ttimes.nim --replace "/usr/share/zoneinfo" "${tzdata}/share/zoneinfo"

In that case we could drop it from nativeBuildInputs. Or otherwise move to checkInputs.

@royneary
Copy link
Contributor Author

Oh, I didn't know checkInputs. The same applies for nodejs-slim-11_x and coreutils then.

@veprbl
Copy link
Member

veprbl commented Jun 20, 2019

@royneary Could you please squash the nim-related commits?
Also might want to fix your commit author email. You could use "royneary@users.noreply.github.com" if you don't want to share your personal address.

@royneary
Copy link
Contributor Author

@veprbl I squashed the commits. Are you suggesting to change my email address because github wasn't able to verify my GPG signatures? I fixed that.

@veprbl
Copy link
Member

veprbl commented Jun 20, 2019

@royneary Thanks! I was mostly worried about the commit attribution to your username.

There is an eval error now. I guess you missing some parenthesis.

@royneary
Copy link
Contributor Author

royneary commented Jun 20, 2019 •
edited

@veprbl fixed. It was because in master someone removed a variable that was then unused but is now used again :-)

@veprbl
Copy link
Member

veprbl commented Jun 21, 2019

@GrahamcOfBorg build nim mosdepth nrpl

veprbl
veprbl reviewed Jun 21, 2019
View reviewed changes
pkgs/development/compilers/nim/default.nix
mv $out/nim/bin/* $out/bin/ && rmdir $out/nim/bin
mv $out/nim/* $out/ && rmdir $out/nim
wrapProgram $out/bin/nim \
--suffix PATH : ${lib.makeBinPath [ stdenv.cc ]}
Copy link
Member

Choose a reason for hiding this comment

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

This wrapper is still needed, I think.

Copy link
Member

Choose a reason for hiding this comment

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

Does "nim compile" work for you in pure environment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it doesnt, you are right. I fixed it.

@royneary
mosdepth: 0.2.3 -> 0.2.6
d162165
@royneary
nim: 0.19.4 -> 0.20.0
b0b64b3 
- link sqlite (needed for the the stdlib sqlite module)
- update nodejs to 11.x
- use default phase order (fixes linker errors during test phase)
- substitution in ./test/async/tioselectors.nim not needed anymore
- two more tests need to be disabled
- LD=$CC workaround not needed anymore
- disable unsupported tests on aarch64
- trigger pre and post hooks
- use checkInputs instead of nativeBuildInputs
- don't override patchPhase
@veprbl
Copy link
Member

veprbl commented Jun 26, 2019

@GrahamcOfBorg build nim mosdepth nrpl

@veprbl veprbl merged commit bff2243 into NixOS:master Jun 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mmahut mmahut mmahut approved these changes

@veprbl veprbl veprbl approved these changes

Assignees
No one assigned
Labels
10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@royneary @veprbl @peterhoeg @mmahut