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

vhd2vl: fix the tests #172443

Merged
merged 1 commit into from May 12, 2022
Merged

vhd2vl: fix the tests #172443

merged 1 commit into from May 12, 2022

Conversation

ghost
Copy link

@ghost ghost commented May 11, 2022

ZHF: #172160

Description of changes

The testing scheme for vhd2vl is sensitive to subtle shifts in
iverilog's parenthesization choices, meaning that the golden test
outputs require constant maintenance.

The patch previously applied in order to deal with this situation is
no longer sufficient, so a patch which is sufficient has been added.

Also, the buildTargets and checkTarget attributes have been set,
so future benign failures of this sort can be dealt with through
doCheck=false in a pinch.

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/)
  • 22.05 Release Notes (or backporting 21.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.

@ofborg ofborg bot requested a review from matthuszagh May 11, 2022 04:01
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild 10.rebuild-linux: 1-10 10.rebuild-linux: 1 labels May 11, 2022
@ghost
Copy link
Author

ghost commented May 11, 2022

The ZHF announcement PR says: "Please ping @NixOS/nixos-release-managers on the PR and add the 0.kind: build failure label to the pull request. If you're unable to because you're not a member of the NixOS org please ping @dasJ, @tomberek, @jonringer, @Mic92"

I am unable to add this build label, so I therefore am issuing this ping.

@Mindavi Mindavi added the 0.kind: build failure A package fails to build label May 11, 2022
@@ -19,12 +19,8 @@ stdenv.mkDerivation rec {
};

patches = lib.optionals (!stdenv.isAarch64) [
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you check aarch64?

Copy link
Author

Choose a reason for hiding this comment

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

Did you check aarch64?

Checking now...

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, the weird behavior (which predates this PR) is confirmed: the patch to the tests must be applied on x86_64 and must not be applied on aarch64.

Apparently the parenthesization algorithm of iverilog's pretty-printer depends on the architecture. Wut.

However figuring out why this hack (which predates this PR) is necessary is probably not something to be done during ZHF.

The testing scheme for vhd2vl is sensitive to subtle shifts in
iverilog's parenthesization choices, meaning that the golden test
outputs require constant maintenance.

The patch previously applied in order to deal with this situation is
no longer sufficient, so a patch which is sufficient has been added.

Also, the `buildTargets` and `checkTarget` attributes have been set,
so future benign failures of this sort can be dealt with through
`doCheck=false` in a pinch.
@ghost ghost requested a review from Mindavi May 12, 2022 07:39
@Mindavi Mindavi merged commit 716bc55 into NixOS:master May 12, 2022
@ghost
Copy link
Author

ghost commented May 13, 2022

Did you check aarch64?

Yeah, the weird behavior (which predates this PR) is confirmed: the patch to the tests must be applied on x86_64 and must not be applied on aarch64.

@Mindavi the following just occurred to me: in light of the fact that this package produces nondeterministically-parenthesized outputs, and has not been updated since 2018, perhaps this would have been a good time to start the process of culling this package from the herd by allowing it to be marked as broken. I realized that ZHF is essentially nixpkgs' mechanism for managing this culling process. Perhaps your comment was a hint to this effect.

If I interfered with that process, I apologize. I have no particular use or need for this package.

Going forward, I'll take a moment to consider whether packages still belong in nixpkgs, before fixing them.

@Mindavi
Copy link
Contributor

Mindavi commented May 13, 2022

No worries, someone else might find this package useful. But generally, if they're broken for a while, it may mean that there are no users and it can at some point be removed.

As long as someone is interested enough (even out of curiosity) to patch a package up, it can stay. If nobody cares for an extended period of time, it's probably not an important package.

But yeah, releases can be a good moment to clean out any old non-important packages, that's definitely true.

I'd say that most people would love to keep everything, it mainly doesn't make sense from a maintenance standpoint to keep supporting everything indefinitely.

Thanks for fixing things! ✨

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0.kind: build failure A package fails to build 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild 10.rebuild-linux: 1-10 10.rebuild-linux: 1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant