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

verilator: set doCheck to true #109493

Closed
wants to merge 2 commits into from
Closed

verilator: set doCheck to true #109493

wants to merge 2 commits into from

Conversation

yangm2
Copy link
Contributor

@yangm2 yangm2 commented Jan 16, 2021

Motivation for this change

Follow-on to #109249 enabling doCheck for Verilator

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

@yangm2
verilator: set doCheck to true
2837c4d 
run the basic compiler/simulator checks
@ofborg ofborg bot requested a review from thoughtpolice January 16, 2021 04:07
@SuperSandro2000
Copy link
Member

This is a semi-automatic executed nixpkgs-review which does not build all packages (e.g. lumo, tensorflow or pytorch)
If you find some bugs or got suggestions for further things to search or run please reach out to SuperSandro2000 on IRC.

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

1 package failed to build and are new build failure:

@SuperSandro2000
Copy link
Member

This is a semi-automatic executed nixpkgs-review which does not build all packages (e.g. lumo, tensorflow or pytorch)
If you find some bugs or got suggestions for further things to search or run please reach out to SuperSandro2000 on IRC.

Result of nixpkgs-review pr 109493 run on x86_64-darwin 1

1 package failed to build and are new build failure:

@thoughtpolice
Copy link
Member

Thanks! This is something I've wanted to get around to. When the CI looks good we can go ahead.

I seem to remember trying to enable this at some point and it looking a little hairy a long time ago, but it might have gotten better. We can also always send patches upstream, too...

@yangm2
Copy link
Contributor Author

yangm2 commented Jan 17, 2021

This is my first time trying to dig deeper into nixpkgs and NixOS ... I'm developing on a Mac/M1. Is iterating through CI the best way? I didn't see a great option for testing this on NixOS locally (VirtualBox?).

@yangm2
Copy link
Contributor Author

yangm2 commented Jan 17, 2021

@thoughtpolice GH says all checks have passed (do we ignore the "neutral checks" that failed?). I don't see any new failure emails from Sandro. Does that mean this is ready for review?

@SuperSandro2000
Copy link
Member

Is iterating through CI the best way?

No, because it takes a long time and especially in big PRs keeps ofborg busy. Testing locally is always preferred.

I didn't see a great option for testing this on NixOS locally (VirtualBox?).

You can use virtualbox or vagrant if you like https://github.com/nix-community/nixbox.

do we ignore the "neutral checks" that failed?

That should be the same or similar checks that failed for me. I honestly don't know why they are neutral but the tests need to pass for amd64 linux at least. For darwin we can disable them.

I don't see any new failure emails from Sandro. Does that mean this is ready for review?

I am just busy with a lot of PRs and didn't have the time to take another look at this.

@SuperSandro2000
Copy link
Member

This is a semi-automatic executed nixpkgs-review which does not build all packages (e.g. lumo, tensorflow or pytorch)
If you find some bugs or got suggestions for further things to search or run please reach out to SuperSandro2000 on IRC.

Result of nixpkgs-review pr 109493 run on x86_64-darwin 1

1 package failed to build and are new build failure:

@stale
Copy link

stale bot commented Jul 21, 2021

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 21, 2021
@yangm2 yangm2 closed this Oct 30, 2022
@thoughtpolice thoughtpolice mentioned this pull request Nov 1, 2022
Merged
8 tasks
@thoughtpolice
Copy link
Member

@yangm2 Hi there, I saw you closed this. Sorry for letting it slide. I was able to look into this when I saw the notification, and also saw that Verilator happily released 5.x recently, so I took the liberty to resurrect your patch to enable the testsuite: see #199068

Sorry for the (super) long wait!

@yangm2
Copy link
Contributor Author

yangm2 commented Nov 2, 2022

Awesome, thanks!

thoughtpolice added a commit to thoughtpolice/nixpkgs that referenced this pull request Nov 2, 2022
@thoughtpolice @yangm2
verilator: enable doCheck = true
a486497 
Based on the work from yangm2 in NixOS#109493.

Co-authored-by: yangm2 <yangm2@users.noreply.github.com>
Signed-off-by: Austin Seipp <aseipp@pobox.com>
thoughtpolice added a commit that referenced this pull request Nov 2, 2022
@thoughtpolice @yangm2
verilator: enable doCheck = true
3afaf7d 
Based on the work from yangm2 in #109493.

Co-authored-by: yangm2 <yangm2@users.noreply.github.com>
Signed-off-by: Austin Seipp <aseipp@pobox.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thoughtpolice thoughtpolice Awaiting requested review from thoughtpolice

Assignees
No one assigned
Labels
2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 10.rebuild-linux: 1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@yangm2 @SuperSandro2000 @thoughtpolice