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

Add a bunch of riscv-related packages #213222

Closed

Conversation

GenericNerdyUsername
Copy link
Contributor

Description of changes

Add riscv-gnu-toolchain, python3Packages.riscof, python3Packages.fusesoc and their dependencies

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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.05 Release Notes (or backporting 22.11 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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@NickCao
Copy link
Member

NickCao commented Jan 29, 2023

May I ask why so many just want riscv-gnu-toolchain instead of using pkgsCross.riscv64.stdenv.cc, is there any specific feature that is not available in the nixpkgs toolchain?

@GenericNerdyUsername
Copy link
Contributor Author

pkgsCross.riscv(64/32).stdenv.cc targets riscv(64/32)-unknown-linux-gnu, riscv-gnu-toolchain targets riscv(64/32)-unkown-elf

@NickCao
Copy link
Member

NickCao commented Jan 29, 2023

What about pkgsCross.riscv64-embedded

@GenericNerdyUsername
Copy link
Contributor Author

GenericNerdyUsername commented Jan 29, 2023

That targets riscv64-none-elf, which I tried to use instead for something, but it didnt work

Copy link
Member

@NickCao NickCao left a comment

Choose a reason for hiding this comment

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

This is a huge PR. Can we split into smaller ones to ease reviewing and merging?

pname = "sail-riscv";
version = "0.5";

nativeBuildInputs = with ocamlPackages; [ sail ocamlbuild findlib ncurses ocaml zlib.dev z3 ];
Copy link
Member

Choose a reason for hiding this comment

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

No need to use zlib.dev here, zlib is enough, stdenv automatically selects the correct output. Also only compilers/tools should reside in nativeBuildInputs, libraries should be in buildInputs. You may set strictDeps = true; to make sure everything is correctly located.


outputs = [ "out" "ocaml_emu" "isabelle_model" "coq_model" "hol4_model" ];

preConfigure = "sed 's/^SAIL:=$(/SAIL:=sail #/' -i Makefile";
Copy link
Member

Choose a reason for hiding this comment

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

set makeFlags = [ "SAIL=sail" ] is normally enough, no need to patch the Makefile.

Comment on lines +33 to +34
SAIL_DIR = "${ocamlPackages.sail}/share/sail";
SAIL = "sail";
Copy link
Member

Choose a reason for hiding this comment

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

Move these to makeFlags if possible.

Comment on lines +37 to +45
runHook preInstall
mkdir -p $out/bin
cp c_emulator/riscv_sim_${arch} $out/bin
mkdir -p $ocaml_emu/bin
cp ocaml_emulator/riscv_ocaml_sim_${arch} $ocaml_emu/bin
cp -r generated_definitions/coq/ $coq_model
cp -r generated_definitions/isabelle/ $isabelle_model
cp -r generated_definitions/hol4/ $hol4_model
runHook postInstall
Copy link
Member

Choose a reason for hiding this comment

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

Are these files extremely large or something? Keeping everything in $out is fine in most cases.

Suggested change
runHook preInstall
mkdir -p $out/bin
cp c_emulator/riscv_sim_${arch} $out/bin
mkdir -p $ocaml_emu/bin
cp ocaml_emulator/riscv_ocaml_sim_${arch} $ocaml_emu/bin
cp -r generated_definitions/coq/ $coq_model
cp -r generated_definitions/isabelle/ $isabelle_model
cp -r generated_definitions/hol4/ $hol4_model
runHook postInstall
runHook preInstall
install -Dm755 c_emulator/riscv_sim_${arch} $out/bin/riscv_sim_${arch}
install -Dm755 ocaml_emulator/riscv_ocaml_sim_${arch} $ocaml_emu/bin/riscv_ocaml_sim_${arch}
cp -r generated_definitions/coq/ $coq_model
cp -r generated_definitions/isabelle/ $isabelle_model
cp -r generated_definitions/hol4/ $hol4_model
runHook postInstall

};
newlib = {
url = newlib.src.url;
hash = "sha256-8pbjcvUTJCJNOHzBFtw3pr05cZh1Z0b5OisC6aXUAVQ=";
Copy link
Member

Choose a reason for hiding this comment

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

(same for spike and pk) Why override the hash here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The original (newlib/spike/pk).src uses fetchurl, not fetchzip, so its just more convenient

Copy link
Member

Choose a reason for hiding this comment

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

However the url could change as the versions are updated, these override would slowly rotten.

Comment on lines +28 to +29
mv $out/bin/lem $out/bin/.lem_wrapped
makeWrapper $out/bin/.lem_wrapped $out/bin/lem --set LEMLIB $out/share/lem/library
Copy link
Member

Choose a reason for hiding this comment

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

Use wrapProgram in stead of makeWrapper, which automatically handles creating .foo-wrapped.

duneVersion = "3";
minimalOCamlVersion = "4.08";

SAIL_DIR = "$out";
Copy link
Member

Choose a reason for hiding this comment

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

Could this be moved to makeFlags (or equivalent for buildDunePackage, I know little about ocaml packaging.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Which part in the project need this ? maybe their is a better way.

Comment on lines +39 to +55
buildPhase = ''
runHook preBuild
rm -r aarch* # Remove code derived from non-bsd2 arm spec
rm -r snapshots # Some of this might be derived from stuff in the aarch dir, it builds fine without it
dune build --release ''${enableParallelBuild:+-j $NIX_BUILD_CORES}
runHook postBuild
'';
checkPhase = ''
runHook preCheck
dune runtest ''${enableParallelBuild:+-j $NIX_BUILD_CORES}
runHook postCheck
'';
installPhase = ''
runHook preInstall
dune install --prefix $out --libdir $OCAMLFIND_DESTDIR
runHook postInstall
'';
Copy link
Member

Choose a reason for hiding this comment

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

Looks like these should have been handled by buildDunePackage?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

buildDunePackage only builds a single package from a source tree, sail has multiple packages in the same source tree

'';
postInstall = ''
mv $out/bin/sail $out/bin/.sail_wrapped
makeWrapper $out/bin/.sail_wrapped $out/bin/sail --set SAIL_DIR $out/share/sail
Copy link
Member

Choose a reason for hiding this comment

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

use wrapProgram

pname = "fusesoc";
version = "1.12.0";

propagatedBuildInputs = [ edalize pyparsing pyyaml simplesat ipyxact verilog verilator gnumake gcc ];
Copy link
Member

Choose a reason for hiding this comment

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

Why are gnumake and gcc in propagated build inputs?

Copy link
Contributor

Choose a reason for hiding this comment

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

it should be in propagatedNativeBuildInputs or nativeBuildInputs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

theyre needed at runtime

Copy link
Contributor

@Et7f3 Et7f3 left a comment

Choose a reason for hiding this comment

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

please add why doCheck are disabled otherwise it looks correct (I haven't tested)

Comment on lines +41 to +42
rm -r aarch* # Remove code derived from non-bsd2 arm spec
rm -r snapshots # Some of this might be derived from stuff in the aarch dir, it builds fine without it
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not define it un preBuild hook ? since you rewrite a part of buildDunePackage

duneVersion = "3";
minimalOCamlVersion = "4.08";

SAIL_DIR = "$out";
Copy link
Contributor

Choose a reason for hiding this comment

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

Which part in the project need this ? maybe their is a better way.

version = "0.55.0";

propagatedBuildInputs = [ mbstrdecoder typepy ];
doCheck = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why disabling test ?

pname = "fusesoc";
version = "1.12.0";

propagatedBuildInputs = [ edalize pyparsing pyyaml simplesat ipyxact verilog verilator gnumake gcc ];
Copy link
Contributor

Choose a reason for hiding this comment

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

it should be in propagatedNativeBuildInputs or nativeBuildInputs

version = "1.1.1";

propagatedBuildInputs = [ chardet ];
doCheck = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why those test are disabled

let

versionFile = writeText "simplesat_ver" ''
# THIS FILE IS GENERATED FROM SETUP_EXT
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add command if someone need to regenerate this ?

buildPythonPackage rec {
pname = "tcolorpy";
version = "0.1.2";
doCheck = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

version = "1.3.0";

propagatedBuildInputs = [ mbstrdecoder python-dateutil pytz packaging ];
doCheck = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

pname = "zipfile2";
version = "0.0.12";

doCheck = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

@@ -32129,7 +32144,7 @@ with pkgs;

eiskaltdcpp = libsForQt5.callPackage ../applications/networking/p2p/eiskaltdcpp { };

qdirstat = libsForQt5.callPackage ../applications/misc/qdirstat {};
qdirstat = libsForQt5.callPackage ../applications/misc/qdirstat { };
Copy link
Contributor

Choose a reason for hiding this comment

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

please avoid unrelated change it is bad for git diff (or do this in a separate commit so it can be in .git-blame-ignore-revs)

@Et7f3
Copy link
Contributor

Et7f3 commented Jan 30, 2023

ofborg doesn't understands sail-riscv-*: prefix could you list them all ? and repush this commit ?

@GenericNerdyUsername
Copy link
Contributor Author

This is a huge PR. Can we split into smaller ones to ease reviewing and merging?

One for riscv-gnu-toolchain, one for python3Packages.riscof and one for python3Packages.fusesoc?

@NickCao
Copy link
Member

NickCao commented Jan 30, 2023

One for riscv-gnu-toolchain, one for python3Packages.riscof and one for python3Packages.fusesoc?

Just do it as you would like to.

@GenericNerdyUsername
Copy link
Contributor Author

ofborg doesn't understands sail-riscv-*: prefix could you list them all ? and repush this commit ?

where do I check this?

@Et7f3
Copy link
Contributor

Et7f3 commented Jan 30, 2023

ofborg doesn't understands sail-riscv-*: prefix could you list them all ? and repush this commit ?

where do I check this?

My bad it seem to have found sail-riscv-rv32 and sail-riscv-rv64 by another means. It didn't find riscv-gnu-toolchain-*

You can check what is found here
image

The general rule of thumb is spell them all separated via comma no brace/abbreviations and it will be fine. So for the first commit sail-riscv-rv32,sail-riscv-rv64: ... or more if more attribute were added

@GenericNerdyUsername
Copy link
Contributor Author

All sub-PRs are done

@GenericNerdyUsername GenericNerdyUsername deleted the more-riscv-pkgs branch February 6, 2023 21:34
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.

3 participants