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

julia_13: fix julia_13 missing Lib of lapack. #89270

Merged
merged 1 commit into from Jun 11, 2020

Conversation

GTrunSec
Copy link
Contributor

@GTrunSec GTrunSec commented May 31, 2020

Motivation for this change
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.

@drewrisinger
Copy link
Contributor

Tests still fail on OfBorg. I'm a little leery of disabling tests in a PR that doesn't include that in the title.

@drewrisinger
Copy link
Contributor

cc @twhitehead who was working on the Julia tests in #79174 for thoughts on this disabling of tests.

@GTrunSec
Copy link
Contributor Author

GTrunSec commented Jun 1, 2020

Tests still fail on OfBorg. I'm a little leery of disabling tests in a PR that doesn't include that in the title.

yeh, we had a discussing in julia_13 PR. #79174 (comment) about disable the test file. In expected testing that is

You either have to do some text substitution on the test file (e.g., use sed to put a # in front of the line that runs the test) or remove or zero the test file itself. I'll submit my suggested cleanups when I get the chance (currently just having problems with one more test that I'm tracking down).

but, Until this time the testing problem still in here. that is why the Julia didn't have binary cache in hydra CI.

Tests still fail on OfBorg.

@GTrunSec
Copy link
Contributor Author

GTrunSec commented Jun 1, 2020

@drewrisinger my own hydra CI test for julia_13 at here. http://221.4.35.244:8300/build/6#tabs-summary

@drewrisinger
Copy link
Contributor

The OfBorg evaluation of this PR with tests fails: https://github.com/NixOS/nixpkgs/pull/89270/checks?check_run_id=725656788. Looks OfBorg build was against julia, not julia_13. Is that the one you meant to build/fix?
@GrahamcOfBorg build julia_13

@drewrisinger
Copy link
Contributor

looks like title should be changed to julia_13: ...

];

enableParallelBuilding = true;

doCheck = !stdenv.isDarwin;
checkTarget = "testall";
checkTarget = "";
Copy link
Contributor

Choose a reason for hiding this comment

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

From your hydra log, it looks like this line just disables all tests. If that's what you want to do, you should change this to doCheck = false, vs obfuscating this by setting checkTarget = "".
I'm not comfortable approving anything that disables ALL tests, especially for something as big as a programming language.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks told me the reason. I will reset hard to HEAD~1. Only adding dependence in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

last build on hydra test

@GTrunSec GTrunSec changed the title julia: fix julia_13 missing Lib of lapack. julia_13: fix julia_13 missing Lib of lapack. Jun 1, 2020
@drewrisinger
Copy link
Contributor

@GrahamcOfBorg eval
@GrahamcOfBorg build julia_13

@twhitehead
Copy link
Contributor

@drewrisinger sorry for the delay in following up with the prior one. I've followed opened up the following upstream bug reports with respect to a variety of issues that are tripping up in nix

I also discovered you need to set HOME and USER to something reasonable or a bunch of tests break

preCheck = ''
  HOME=$NIX_BUILD_TOP
  USER=$(whoami)
  JULIA_CPU_THREADS=''${NIX_BUILD_CORES:-1}
''

@drewrisinger
Copy link
Contributor

@twhitehead thanks for the response, I think fixing Julia tests is now out of scope for this PR, though I think it's still very important.

@twhitehead
Copy link
Contributor

The underlying issue is that the build runs multiple tests in a single julia instance and not all tests leave behind a clean state (i.e., some of the earlier tests break things for later tests).

Why this is showing up under nix and not most other systems is because building without a network connection causes all the test to run serially in one julia process (where all bad test interactions surface) compared to the standard way of running tests in parallel across multiple julia process (where bad test interactions only surface if they happen to occur in the same julia process).

So, yeah, the Julia test process as it stands is reasonably broken. At least now upstream has a reproducible way to trigger it (test with only one process) and will hopefully get stuff cleaned up.

Copy link
Contributor

@drewrisinger drewrisinger left a comment

Choose a reason for hiding this comment

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

Diff LGTM.
With nix-review pr 89270, doesn't build. Seems to be b/c of upstream test failures. However, this is a strict improvement and it would likely build if tests disabled. See comments in thread for status of Julia testing in Nix @twhitehead.
Fixes fallout from #83888

@GTrunSec
Copy link
Contributor Author

GTrunSec commented Jun 3, 2020

@twhitehead. @cstich I bumped up julia_13 to julia1.4.2. also, I did a build test and patched you have mentioned code on hydra CI. there are my test logs.http://221.4.35.244:8300/build/36/nixlog/1
that will help you to debug in testall process.

@7c6f434c 7c6f434c merged commit d65bcef into NixOS:master Jun 11, 2020
@GTrunSec GTrunSec deleted the julia-lapack branch June 11, 2020 20:35
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.

None yet

4 participants