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

python3Packages.riscof: init 1.25.2 #213832

Merged
merged 14 commits into from
Feb 9, 2023
Merged

Conversation

GenericNerdyUsername
Copy link
Contributor

Description of changes

Add riscof (and 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.

Relevant: #213222

@Et7f3
Copy link
Contributor

Et7f3 commented Feb 1, 2023

Did you applied suggestion on the other PR ?

@GenericNerdyUsername
Copy link
Contributor Author

Yup

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.

Quite a lot formatting and code style issues. You can use nixpkgs-format, statix and other tools you help you locate/fix these issues.

pkgs/applications/virtualization/sail-riscv/default.nix Outdated Show resolved Hide resolved
pkgs/applications/virtualization/sail-riscv/default.nix Outdated Show resolved Hide resolved
pkgs/applications/virtualization/sail-riscv/default.nix Outdated Show resolved Hide resolved
pkgs/applications/virtualization/sail-riscv/default.nix Outdated Show resolved Hide resolved
pkgs/development/ocaml-modules/lem/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/dataproperty/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/dataproperty/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/mbstrdecoder/default.nix Outdated Show resolved Hide resolved
@GenericNerdyUsername
Copy link
Contributor Author

All the problems mentioned in the review should be fixed now

pkgs/applications/virtualization/sail-riscv/default.nix Outdated Show resolved Hide resolved
pkgs/development/ocaml-modules/lem/default.nix Outdated Show resolved Hide resolved
pkgs/development/ocaml-modules/sail/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/pytablewriter/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/riscv-isac/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/typepy/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/typepy/default.nix Outdated Show resolved Hide resolved
@GenericNerdyUsername
Copy link
Contributor Author

Assuming fixing the merge conflict didn't break anything, I think this is ready to merge (unless there's something in the review i missed?)

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.

looks good to my eye, just need someone who knows the ocaml ecosystem better than me to take a second look.

@NickCao NickCao requested a review from vbgl February 8, 2023 01:03
@NickCao
Copy link
Member

NickCao commented Feb 8, 2023

@vbgl sorry for pinging you without asking for permission, but you seem to be working with ocaml things recently and I hope to get a review from you.

@vbgl
Copy link
Contributor

vbgl commented Feb 8, 2023

No problem. I’ll try to give a look at the OCaml parts of this PR.

Copy link
Contributor

@vbgl vbgl left a comment

Choose a reason for hiding this comment

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

Most of the comments/suggestion also apply to the linksem library.

pkgs/development/ocaml-modules/lem/default.nix Outdated Show resolved Hide resolved
pkgs/development/ocaml-modules/lem/default.nix Outdated Show resolved Hide resolved
pkgs/development/ocaml-modules/lem/default.nix Outdated Show resolved Hide resolved
pkgs/development/ocaml-modules/lem/default.nix Outdated Show resolved Hide resolved
pkgs/development/ocaml-modules/lem/default.nix Outdated Show resolved Hide resolved
@GenericNerdyUsername
Copy link
Contributor Author

@vbgl can you check if i missed any of your recommendations?

@GenericNerdyUsername
Copy link
Contributor Author

Done

@NickCao
Copy link
Member

NickCao commented Feb 9, 2023

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

20 packages built:
  • python310Packages.dataproperty
  • python310Packages.mbstrdecoder
  • python310Packages.pytablewriter
  • python310Packages.riscof
  • python310Packages.riscv-config
  • python310Packages.riscv-isac
  • python310Packages.tabledata
  • python310Packages.tcolorpy
  • python310Packages.typepy
  • python311Packages.dataproperty
  • python311Packages.mbstrdecoder
  • python311Packages.pytablewriter
  • python311Packages.riscof
  • python311Packages.riscv-config
  • python311Packages.riscv-isac
  • python311Packages.tabledata
  • python311Packages.tcolorpy
  • python311Packages.typepy
  • sail-riscv-rv32
  • sail-riscv-rv64

@NickCao
Copy link
Member

NickCao commented Feb 9, 2023

@GenericNerdyUsername thanks for your contribution and patience in addressing review comments.
@vbgl also thanks for your review.

@NickCao NickCao merged commit aa06096 into NixOS:master Feb 9, 2023
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.

4 participants