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

llvm: add in a missing check dep #218865

Merged
merged 1 commit into from
Mar 10, 2023
Merged

llvm: add in a missing check dep #218865

merged 1 commit into from
Mar 10, 2023

Conversation

alyssais
Copy link
Member

@alyssais alyssais commented Feb 28, 2023

Description of changes

Port of 6d0c876 ("llvmPackages_15.llvm: add in a missing check dep").

I did not include the sysctl nativeCheckInputs change from that commit, as it looks like it was included by mistake and was supposed to be in c7231c0 ("llvmPackages_15.llvm: run the tests on macOS"). I've also included the doCheck default from that commit (without the change to run on non-Linux), as 6d0c876 just set it to true.

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
  • Fits CONTRIBUTING.md.

Port of 6d0c876 ("llvmPackages_15.llvm: add in a missing check
dep").

I did not include the sysctl nativeCheckInputs change from that
commit, as it looks like it was included by mistake and was supposed
to be in c7231c0 ("llvmPackages_15.llvm: run the tests on macOS").
I've also included the doCheck default from that commit (without the
change to run on non-Linux), as 6d0c876 just set it to true.
Copy link
Contributor

@rrbutani rrbutani left a comment

Choose a reason for hiding this comment

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

Apologies for the delay.


I did not include the sysctl nativeCheckInputs change from that commit, as it looks like it was included by mistake and was supposed to be in c7231c0 ("llvmPackages_15.llvm: run the tests on macOS").

I've also included the doCheck default from that commit (without the change to run on non-Linux), as 6d0c876 just set it to true.

Ah, thanks for untangling that; I definitely bungled the commits in that PR a bit.


This looks good!

Just to confirm: are these changes intentionally not backported to llvmPackages_5? I haven't tried running the test suite to verify but a quick peek suggests that the dep was present in lit even then.

Comment on lines +18 to +19
, doCheck ? stdenv.isLinux && (!stdenv.isx86_32) && (!stdenv.hostPlatform.isMusl) && (!stdenv.hostPlatform.isRiscV)
&& (stdenv.hostPlatform == stdenv.buildPlatform)
Copy link
Contributor

Choose a reason for hiding this comment

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

Just in case anyone else was also wondering: #211728 copies this condition (which LLVM 9 and 11 have) and disables the tests on RISC-V hosts for LLVM 12 - 14 (though not LLVM 10...); it'll have to be adjusted once this PR is merged.

@alyssais
Copy link
Member Author

alyssais commented Mar 9, 2023

Just to confirm: are these changes intentionally not backported to llvmPackages_5? I haven't tried running the test suite to verify but a quick peek suggests that the dep was present in lit even then.

It was intentional, as in my testing it didn't emit the warning message. I could try testing if the actual number of tests run changes.

@alyssais
Copy link
Member Author

LLVM 6, with psutil:

/nix/store/ljasfl681ldyq7rwj0df94avyzib8kmi-python3-3.10.10-env/bin/python /build/llvm/build/./bin/llvm-lit -svj64 --no-progress-bar /build/llvm/build/utils/lit /build/llvm/build/test
llvm-lit: /build/llvm/build/utils/lit/tests/lit.cfg:58: note: Found python psutil module
-- Testing: 23291 tests, 64 threads --

Testing Time: 57.35s
  Expected Passes    : 22432
  Expected Failures  : 140
  Unsupported Tests  : 719

LLVM 6, without psutil:

/nix/store/grmc7s8xvc6khbgs0g0rkssc883wwncx-python3-3.10.10/bin/python /build/llvm/build/./bin/llvm-lit -svj64 --no-progress-bar /build/llvm/build/utils/lit /build/llvm/build/test
llvm-lit: /build/llvm/build/utils/lit/tests/lit.cfg:61: warning: Could not import psutil. Some tests will be skipped and the --timeout command line argument will not work.
-- Testing: 23291 tests, 64 threads --

Testing Time: 79.47s
  Expected Passes    : 22430
  Expected Failures  : 140
  Unsupported Tests  : 721

1 warning(s) in tests.

LLVM 5, with psutil:

/nix/store/ljasfl681ldyq7rwj0df94avyzib8kmi-python3-3.10.10-env/bin/python /build/llvm/utils/lit/lit.py -svj64 --no-progress-bar --param llvm_site_config=/build/llvm/build/test/lit.site.cfg --param llvm_unit_site_config=/build/llvm/build/test/Unit/lit.site.cfg /build/llvm/build/test
-- Testing: 21448 tests, 64 threads --

Testing Time: 22.92s
  Expected Passes    : 20627
  Expected Failures  : 131
  Unsupported Tests  : 690

LLVM 5, without psutil:

/nix/store/grmc7s8xvc6khbgs0g0rkssc883wwncx-python3-3.10.10/bin/python /build/llvm/utils/lit/lit.py -svj64 --no-progress-bar --param llvm_site_config=/build/llvm/build/test/lit.site.cfg --param llvm_unit_site_config=/build/llvm/build/test/Unit/lit.site.cfg /build/llvm/build/test
-- Testing: 21448 tests, 64 threads --

Testing Time: 22.24s
  Expected Passes    : 20627
  Expected Failures  : 131
  Unsupported Tests  : 690

So it looks to me like LLVM 5, unlike LLVM 6, doesn't skip any tests as "unsupported" if psutil is missing.

@alyssais alyssais merged commit 3816765 into NixOS:staging Mar 10, 2023
@alyssais alyssais deleted the llvm-psutil branch March 10, 2023 12:21
@rrbutani rrbutani added the 6.topic: llvm/clang Issues related to llvmPackages, clangStdenv and related label May 27, 2024
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.

3 participants