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

Begin supporting nix flakes build system #4238

Merged
merged 9 commits into from
May 21, 2024
Merged

Conversation

RCoeurjoly
Copy link
Contributor

@RCoeurjoly RCoeurjoly commented Feb 25, 2024

So far there is:

  • a default package called yosys
  • a devShell to create a environment with everything necesary to compile yosys
  • A github workflow to build yosys (nix-github-actions.yml)
  • A github workflow to update flake.lock (update-flake-lock.yml)

Follow up actions would be to:

  • include tests in flake.nix

Copy link
Member

@whitequark whitequark left a comment

Choose a reason for hiding this comment

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

I'm not the one with the final say on this PR, but I think it should include a GitHub Actions job that verifies that the flake continues to build.

How often do you have to update the lockfile?

@povik
Copy link
Member

povik commented Feb 25, 2024

Does it make sense to have a flake without the lock file? Is that even possible?

@RCoeurjoly
Copy link
Contributor Author

Does it make sense to have a flake without the lock file? Is that even possible?

I think it doesn't make much sense, because one of the main purposes of nix is to provide reproducibility.
Without the lock file, every time a user builds the package, a new version of nixpkgs would get pulled. Most notably, the version of the compilers and dependencies would not be version controlled.

That's why this PR provides both flake.nix and flake.lock

@povik
Copy link
Member

povik commented Feb 25, 2024

I think it doesn't make much sense, because one of the main purposes of nix is to provide reproducibility. Without the lock file, every time a user builds the package, a new version of nixpkgs would get pulled. Most notably, the version of the compilers and dependencies would not be version controlled.

I understand the value of reproducibility, but I am not convinced we want to maintain a pinned version of compilers and other dependencies as part of the source tree. I worry that would mean a collection of stale hashes which will get updated irregularly.

If we only include the flake without the lock file, that would still mean others can supply their lock files easily, if they wish to pin the dependencies and establish reproducibility. We will just supply the Nix building instructions as part of the source tree.

@RCoeurjoly
Copy link
Contributor Author

I think it doesn't make much sense, because one of the main purposes of nix is to provide reproducibility. Without the lock file, every time a user builds the package, a new version of nixpkgs would get pulled. Most notably, the version of the compilers and dependencies would not be version controlled.

I understand the value of reproducibility, but I am not convinced we want to maintain a pinned version of compilers and other dependencies as part of the source tree. I worry that would mean a collection of stale hashes which will get updated irregularly.

If we only include the flake without the lock file, that would still mean others can supply their lock files easily, if they wish to pin the dependencies and establish reproducibility. We will just supply the Nix building instructions as part of the source tree.

Updating the lock file is easy with nix flake update (https://nixos.org/manual/nix/stable/command-ref/new-cli/nix3-flake-update.html). You can even update one input only.

If no lock file is committed to the repo, and there is github action for building with nix, that building process would pull a different nixpkgs every time, because nixpkgs (https://github.com/NixOS/nixpkgs) gets a lot of activity.

For example at the time of writing this comment, the last commit was only 46 minutes ago.

So I would propose committing the lock file, and maybe creating a bot that updates the lock from time to time?

@whitequark
Copy link
Member

The key question is: What happens when the build breaks?

If YosysHQ has someone who can and will fix the build, that makes it no different than any other source of breakage, whether it's broken immediately after a nixpkgs update, or at night when the cronjob runs. (For example upstream changes in iverilog recently broke our builds.)

If YosysHQ does not, then the flake should probably just not be in the repo.

@whitequark
Copy link
Member

Actually, one advantage of having the flake lockfile be committed that I think no one brought up yet is that it enables people to keep building historical revisions of Yosys in the exact environment that passed CI.

@povik Is this something that will change how you feel about committing the lockfile?

@RCoeurjoly
Copy link
Contributor Author

I have created a repo to maintain the flake out of tree: https://github.com/RCoeurjoly/yosys_flake/tree/main.

I think @whitequark is right in that adding flake to this repo would create a burden on YosysHQ maintainers, so better to maintain it out of tree, kind of like how yosys is in nixpkgs right now.

Thoughts?

@povik
Copy link
Member

povik commented Feb 26, 2024

We have discussed this among Yosys maintainers and have come up with a plan to:

  • do commit the lock file into the repo
  • have two Nix CI jobs, one building with the version-controlled lock file and the other with the latest version of inputs
  • adapt the automation we already have for periodic "Bump version" commits to also update the lock file to a version it can pull from a successfully-completed CI job

@RCoeurjoly RCoeurjoly force-pushed the nix branch 2 times, most recently from 4ad1c52 to bee2ad4 Compare February 26, 2024 19:04
@povik
Copy link
Member

povik commented Feb 26, 2024

We have discussed this among Yosys maintainers and have come up with a plan to:

Just to be sure @RCoeurjoly, I am not asking you to implement all of it, @mmicko and I can look into some of it too, though any work you would do towards the plan is welcome, and would bring the flake getting merged closer.

@RCoeurjoly
Copy link
Contributor Author

We have discussed this among Yosys maintainers and have come up with a plan to:

  • do commit the lock file into the repo
  • have two Nix CI jobs, one building with the version-controlled lock file and the other with the latest version of inputs
  • adapt the automation we already have for periodic "Bump version" commits to also update the lock file to a version it can pull from a successfully-completed CI job

Sounds reasonable.
One problem I am encountering when trying to build yosys with flakes is the following.

[100%] Building abc/abc-896e5e7
Pulling ABC from https://github.com/YosysHQ/abc:

This behavior from nix is expected, since nix tries to be hermetic and one of the parts of that is not accessing the network, because cloning a repo at two different time points would result in two different packages, or derivations (in nix lingo).

Options available of the top of my head:

  • Add YosysHQ/abc as a flake input
  • Add YosysHQ/abc as a submodule

@whitequark
Copy link
Member

  • Add YosysHQ/abc as a submodule

It should be a submodule. It only isn't a submodule because abc used to be version controlled in Mercurial, which (obviously) prevented that.

I once tried to convert it to a submodule but some other horrors in the Yosys Makefile caused me to waste three days on it, after which I swore I will never attempt to improve Yosys Makefile again. I wish you luck.

@povik
Copy link
Member

povik commented Feb 26, 2024

Options available of the top of my head:

  • Add YosysHQ/abc as a flake input

  • Add YosysHQ/abc as a submodule

While it may make sense to make abc a submodule from a conceptual sense, I think this is too much of change affecting downstream users of Yosys to couple it to the work on the Nix flake.

If abc can be made an input to the flake, I would go with that for now.

@whitequark
Copy link
Member

While it may make sense to make abc a submodule from a conceptual sense, I think this is too much of change affecting downstream users of Yosys to couple it to the work on the Nix flake.

I disagree with this reasoning. I think it should have been made a submodule years ago instead of whatever the hell it is right now. The current situation has been negatively affecting users of Yosys for long enough that it should just be fixed, and the current time is as good as any.

@povik
Copy link
Member

povik commented Feb 26, 2024

I suppose we shouldn't not try fixing the status quo because it is the status quo, but it seems valid not to want to couple this to another large task.

@whitequark
Copy link
Member

@RCoeurjoly How do you feel about converting abc to a submodule?

@povik Generally speaking yes, but it's not like we're under pressure to ship flakes ASAP.

@RCoeurjoly
Copy link
Contributor Author

@RCoeurjoly How do you feel about converting abc to a submodule?

@povik Generally speaking yes, but it's not like we're under pressure to ship flakes ASAP.

Yes, my strong preference would be for abc to be a submodule, because it would enable us to compile inside the devShell. Otherwise, if abc is a flake input, working inside the devShell would be very inconvenient.

@whitequark
Copy link
Member

whitequark commented Feb 26, 2024

Then I'd say let's make it into a submodule. I will have to redo a bunch of my build scripts and I will do it with joy.

(I think mainly just replacing git submodule update with git submodule update -r really...)

@povik
Copy link
Member

povik commented Feb 26, 2024

No contest here. Roland, if you want to tackle this, then please go ahead!

@RCoeurjoly RCoeurjoly marked this pull request as draft February 26, 2024 20:42
@whitequark
Copy link
Member

I think adding abc as a submodule would be best done in another PR.

@mmicko
Copy link
Member

mmicko commented Feb 27, 2024

Please be aware that use case where user downloads abc.tar.gz from release page and unpacks it in abc directory should also continue to work.

@widlarizer
Copy link
Collaborator

widlarizer commented Apr 4, 2024

I've written my own shell.nix before noticing this PR. I notice some differences between it and this flake:

  • I only use clang stdenv, I don't explicitly get pkgs.clang, which may be why I don't need libcxx (but this may just be a shell.nix vs flake thing)
  • I have some extra packages bc I just copied deps from CI or docs, namely, gawk, pkg-config, boost
  • For show to work, I have graphviz and xdot
  • To build docs, I have libfaketime pdf2svg texliveFull (python3.withPackages (ps: [ps.furo ps.sphinxcontrib-bibtex]))

I have tested this with nix-shell --pure to verify it still builds yosys & docs succesfully

@RCoeurjoly
Copy link
Contributor Author

I've written my own shell.nix before noticing this PR. I notice some differences between it and this flake:

  • I only use clang stdenv, I don't explicitly get pkgs.clang, which may be why I don't need libcxx (but this may just be a shell.nix vs flake thing)
  • I have some extra packages bc I just copied deps from CI or docs, namely, gawk, pkg-config, boost
  • For show to work, I have graphviz and xdot
  • To build docs, I have libfaketime pdf2svg texliveFull (python3.withPackages (ps: [ps.furo ps.sphinxcontrib-bibtex]))

I have tested this with nix-shell --pure to verify it still builds yosys & docs succesfully

How do you deal with the abc dependency?
When trying to create the flake, it was decided that it was better to convert abc to a submodule.

I am working on that here #4243

@widlarizer
Copy link
Collaborator

I didn't deal with it probably because I built it first with an impure shell which pulled in the dependency. I retried with a pure shell in a fresh checkout and it does indeed fail. I'm definitely in favor of a submodule

@RCoeurjoly RCoeurjoly marked this pull request as ready for review May 14, 2024 15:39
@RCoeurjoly RCoeurjoly requested a review from mmicko as a code owner May 14, 2024 15:39
@nakengelhardt nakengelhardt merged commit d2586e2 into YosysHQ:main May 21, 2024
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants