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

Improve release-cross tests #21414

Merged
merged 4 commits into from Dec 29, 2016
Merged

Conversation

Ericson2314
Copy link
Member

Ahead of #21388 or #21268, it would be nice to test cross-compiling better. This PR cleans up the existing tests, and adds some new ones. Those new ones fail, but that's OK: they won't hold up any channel, and they'll remind us to merge one of the two PRs above, which fixes the new tests.

@DavidEGrayson What do you think? I'd add you as a reviewer on the side panel, but It won't let me. If you like this I'll go ahead and merge it.

@mention-bot
Copy link

@Ericson2314, thanks for your PR! By analyzing the history of the files in this pull request, we identified @edolstra, @domenkozar and @rbvermaa to be potential reviewers.

@Ericson2314 Ericson2314 changed the title Release cross Improve release-cross tests Dec 25, 2016
@Ericson2314 Ericson2314 self-assigned this Dec 25, 2016
@Ericson2314 Ericson2314 added this to the 17.03 milestone Dec 25, 2016
@Ericson2314 Ericson2314 added 6.topic: cross-compilation Building packages on a different sort platform than than they will be run on 0.kind: enhancement 8.has: clean-up labels Dec 25, 2016
Copy link
Contributor

@DavidEGrayson DavidEGrayson left a comment

Choose a reason for hiding this comment

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

The first commit was a little hard to review so I only glanced over it. The rest looks good, though I did point out one nit.


testEqual = path: systems: forAllSupportedSystems systems (testEqualOne path);

mapTestEqual = lib.mapAttrsRecursive (path: systems: testEqual path systems);
Copy link
Contributor

Choose a reason for hiding this comment

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

The expression in parentheses is really just a verbose way of writing testEqual.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh hah! A few refactors in and I totally missed that worthless eta expansion. Thanks!

These derivations do not care about the target platform, and thus should
not be affected by cross-compiling. Currently, these tests *fail*, but they
will be fixed soon by a latter PR. The release-cross job doesn't block a
channel, so this should be no problem.
@Ericson2314 Ericson2314 merged commit 614ae8f into NixOS:master Dec 29, 2016
@Ericson2314 Ericson2314 deleted the release-cross branch December 29, 2016 17:59
@Ericson2314
Copy link
Member Author

http://hydra.nixos.org/eval/1317573 The good news is I didn't break any existing jobs, the bad news is that my assert ...; trues are not considered jobs.

@DavidEGrayson
Copy link
Contributor

Ah, I guess it has to be assert ...; someTrivialJob. But I don't really know how Hydra works.

@Ericson2314
Copy link
Member Author

Ericson2314 commented Dec 29, 2016

Those assertions should be failing, so I figured Hydra would have no idea whether there is a derivation behind them. Maybe it has some magic, but in any event I don't want the job to dissapear if the assert isn't thrown. I'll try something like if .... then "passing-job" runCommand {} "touch $out" else runCommand "failing-job" {} "false"

@dezgeg
Copy link
Contributor

dezgeg commented Dec 29, 2016

FWIW they do show up in http://hydra.nixos.org/jobset/nixpkgs/cross-trunk#tabs-errors. But yes, you do need some sort of dummy derivations so they show up like the others.

@Ericson2314
Copy link
Member Author

Oh, that's a good start. It's weird that that tab is there with all the evaluations, rather than per-evaluation.

@DavidEGrayson
Copy link
Contributor

DavidEGrayson commented Jan 4, 2017

From this PR:

Converting to a string (drv path) before checking equality is probably a
good idea lest there be some irrelevant pass-through debug attrs that
cause false negatives.

Equality comparison of derivations seems to be special and different from equality comparison of a normal set. When you make the changes necessary to get those tests to pass, it proves that a derivation with a crossDrv inside it (for foosys and foolibc) can be equal to a derivation without a crossDrv. So I bet that debugging attributes also don't matter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0.kind: enhancement 6.topic: cross-compilation Building packages on a different sort platform than than they will be run on 8.has: clean-up
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants