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

llvmPackages_git.*: Bump to newer commit #154465

Merged
merged 1 commit into from
Feb 27, 2022
Merged

llvmPackages_git.*: Bump to newer commit #154465

merged 1 commit into from
Feb 27, 2022

Conversation

Ericson2314
Copy link
Member

Motivation for this change

Not sure whether we want to merge this now, or wait for more patches to land upstream first, but it was good to at least confirm the CMake is still working

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 added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Jan 11, 2022
sternenseemann
sternenseemann reviewed Jan 11, 2022
View reviewed changes
Copy link
Member

@sternenseemann sternenseemann left a comment

Choose a reason for hiding this comment

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

The question whether to update or not to, I'd say depends on how much hassle it is for you to rebase the patches etc.?

Haven't looked at the patches or tried building yet, can take a closer look later.

pkgs/development/compilers/llvm/git/libcxx/default.nix
@@ -10,12 +12,26 @@
, headersOnly ? false
}:

let
basename = "libcxx";
Copy link
Member

Choose a reason for hiding this comment

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

Nit: it's basename here and baseName in compiler-rt.

pkgs/development/compilers/llvm/git/libcxx/default.nix
inherit version;

inherit src;
sourceRoot = "source/libcxx";
src = runCommand "${pname}-src-${version}" {} ''
Copy link
Member

Choose a reason for hiding this comment

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

Can we maybe generalize this runCommand thing and replace it with some kind of function (passed in from the top level) which gets a dir and a list of extra paths?

I don't think we can use filterSource for this sadly though.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I think that would be good, just didn't do it yet. I did something similar in #82131 i should perhaps pull out.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that looks right!

pkgs/development/compilers/llvm/git/llvm/default.nix
@@ -22,7 +23,7 @@
|| stdenv.isAarch32 # broken for the armv7l builder
)
, enablePolly ? false
}:
} @args:
Copy link
Member

Choose a reason for hiding this comment

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

args is unused.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah thanks, yeah that was before I renamed src to monorepoSrc.

Copy link
Member

Choose a reason for hiding this comment

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

Removing it in #268812

@primeos
Copy link
Member

primeos commented Jan 11, 2022

Thanks a lot @Ericson2314 and @sternenseemann! :) Unfortunately, llvmPackages_git was getting quite outdated.

Not sure whether we want to merge this now, or wait for more patches to land upstream first, but it was good to at least confirm the CMake is still working

I wouldn't mind merging it. The patches situation isn't ideal (not to say can be a bit frustrating at times :P) but I guess for LLVM it should be fine (and this doesn't make it worse anyway, it just adds a bit more content to Nixpkgs). Hopefully the rest of the big patches will land soon... :)

Anyway: @Ericson2314 and @sternenseemann wouldn't you be interested to add yourself officially as LLVM package maintainers? AFAIK you're currently doing by far most of the work anyway and usually end up reviewing other PRs as well. So it seems like that would really make sense, unless you don't want to for some reason. (AFAIK most listed LLVM maintainers aren't that active btw, e.g., I'm only in there since I ended up having to add new versions for Chromium.)

@Ericson2314
Copy link
Member Author

Yeah I think adding us would make sense.

@primeos
Copy link
Member

primeos commented Feb 17, 2022 •
edited
Loading

Can we merge this now so that we can package LLVM 14.0.0-rc1? (Probably makes sense to pick a revision that doesn't require additional changes for 14.0.0-rc1 (e.g., that exact revision or the commit where LLVM 14 was branched off from master) but maybe that's already the case for the current revision.)

I'm a bit late as it looks like it's already required for Chromium M99 which will be released on Mar 1.
Currently the build generates a binary that fails due to "Program received signal SIGILL, Illegal instruction." I haven't looked into that yet but a newer LLVM version could(/should) fix it. IIRC relevant links are:

@primeos primeos marked this pull request as ready for review February 27, 2022 17:37
@primeos primeos merged commit 6dc6d8c into NixOS:master Feb 27, 2022
@primeos
Copy link
Member

primeos commented Feb 27, 2022 •
edited
Loading

llvmPackages_git.stdenv builds with #162101 but some other packages are broken - it'll have to do for now.
It does fix the chromiumBeta issue.

@Ericson2314 Ericson2314 deleted the bump-llvm-git branch February 27, 2022 22:11
@Ericson2314 Ericson2314 mentioned this pull request Mar 25, 2022
Merged
13 tasks
@primeos primeos mentioned this pull request Apr 30, 2022
Open
@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
Reviewers

@sternenseemann sternenseemann sternenseemann left review comments

@Artturin Artturin Artturin left review comments

@primeos primeos Awaiting requested review from primeos

@matthewbauer matthewbauer Awaiting requested review from matthewbauer

Assignees
No one assigned
Labels
6.topic: llvm/clang Issues related to llvmPackages, clangStdenv and related 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Ericson2314 @primeos @sternenseemann @Artturin @rrbutani