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_11: build from git #114828

Closed
kvtb opened this issue Mar 2, 2021 · 15 comments · Fixed by #120780
Closed

llvm_11: build from git #114828

kvtb opened this issue Mar 2, 2021 · 15 comments · Fixed by #120780
Labels
0.kind: packaging request Request for a new package to be added 6.topic: llvm/clang Issues related to llvmPackages, clangStdenv and related

Comments

@kvtb
Copy link
Contributor

kvtb commented Mar 2, 2021

llvm is currenly built from individual tarballs instead of upstream monorepo

The flaws of this approach are:

  1. there are boilerplate to reconstruct fragments of git monorepo in every subproject's .nix file. For example
    unpackPhase = ''
    unpackFile $src
    mv clang-* clang
    sourceRoot=$PWD/clang
    unpackFile ${clang-tools-extra_src}
    mv clang-tools-extra-* $sourceRoot/tools/extra
    '';
  2. there are difficulties with cherry-picking upstream patches because directory structure is altered, so nixpkgs has to host .patch-files instead of using fetchpatch from upstream git repo: https://github.com/NixOS/nixpkgs/tree/9ddb9c0021ede97e981c3dee2250bbd1abc50c6f/pkgs/development/compilers/llvm/11
  3. it is not possible to use bleeding edge features which are already on git's master but not yet in the latest tarballed release. chromiumDev often needs them. With git repo, llvm_11.override{src = ...} would work.
@kvtb kvtb added the 0.kind: packaging request Request for a new package to be added label Mar 2, 2021
@kvtb
Copy link
Contributor Author

kvtb commented Mar 3, 2021

In upstream (and in some Linux distros) there are already LLVM 12 (12.0.0-rc2 tag) and LLVM 13 (master branch).

NixOS does not have to provide them out-of-the-box.

Just switching to them by .override{src=} would be very nixy.

@kvtb
Copy link
Contributor Author

kvtb commented Mar 3, 2021

related (attempt to customize LLVM): #101786

@kvtb kvtb mentioned this issue Mar 17, 2021
@primeos
Copy link
Member

primeos commented Mar 30, 2021

Also +1 for this btw. I'd be in favor of merging such an implementation as e.g. llvmPackages_git. That way it also wouldn't have to immediately replace the existing implementation (could be improved and extended incrementally) which should make the initial PR easier (focusing e.g. on just clang, lld, and their dependencies as well as a nice and clean diff). Unfortunately I don't have enough time to work on this myself (but I might be able to help a bit).

Documentation and references:

@attila-lendvai
Copy link
Contributor

i don't want to hijack this, but i think it's relevant: i arrived here while looking for a way to get a bleeding edge LLVM/Clang for a project that i'm trying to build (https://github.com/clasp-developers/clasp).

it would be very helpful for us if there were new hydra builds of llvm HEAD every couple of weeks.

sometimes we even need an LLVM with a patch of ours, and i'm looking into setting up a local nix cache for that, but it's not a pressing issue right now (we have no pending LLVM patches at the moment).

shall i hold my breath for an llvmPackages_git? :) or llvmPackages_13 that gets updated every once in a while to follow LLVM's git head?

@primeos
Copy link
Member

primeos commented Apr 14, 2021

shall i hold my breath for an llvmPackages_git? :)

Likely not, we're unfortunately lacking LLVM maintainers. It would certainly be nice though.

it would be very helpful for us if there were new hydra builds of llvm HEAD every couple of weeks.

An additional issue is to decide when to update llvmPackages_git. The best solution that I know of would be to automate this and base it on the version Chromium Dev is currently using:

Chromium ships a prebuilt clang binary. It's just upstream clang built at a known-good revision that we bump every two weeks or so. (Source: https://chromium.googlesource.com/chromium/src/+/master/docs/clang.md)

That way it would be clear when and to which revision to update LLVM and that revision should already be well tested (known-good).

See https://github.com/NixOS/nixpkgs/pull/101786/files#diff-8f208f3301c344e2c9bc9020c2b20c6585c642e8a9601a25669b2998da1c1247R43 for an example to extract this information and there are regular Chromium Dev releases: https://chromiumdash.appspot.com/releases?platform=Linux

I guess I could even get llvmPackages_git going based on llvmPackages_12 but we"d really need someone to actually maintain it (i.e. regularly update it, fix build failures/regressions, keep the Nix expressions maintained, etc.).

@attila-lendvai
Copy link
Contributor

attila-lendvai commented Apr 14, 2021

@primeos what's wrong with just pushing it, and then whenever bitrot happens wait for the opensource fairies to open a PR?

i'm rather new to nix, but i'd certainly keep an eye on it while working on clasp, and do my best to fix things if they break.

is there a way to mark a package "non-essential", or "optional"? i.e. failing to build shouldn't cause any blockage. llvmPackages_git would be a good candidate. (that is, unless it would be used for building chromium)

@primeos
Copy link
Member

primeos commented Apr 14, 2021

@primeos what's wrong with just pushing it, and then whenever bitrot happens wait for the opensource fairies to open a PR?

It's an option but the probability to get problematic regressions is much higher, people might disagree on which revisions to pick, when / how often to update, etc. That's why I'd prefer to stick to a clear plan (also e.g. our gn in Nixpkgs is based on master and IIRC it basically bitrots until something breaks and someone updates it) - it's not required but I fell like it'd work much better.

is there a way to mark a package "non-essential", or "optional"? i.e. failing to build shouldn't cause any blockage. llvmPackages_git would be a good candidate. (that is, unless it would be used for building chromium)

It would be non-essential by default so it should be fine. It might even be best to add the attribute via pkgs/top-level/aliases.nix so that nothing in Nixpkgs will "accidentally" depend on it (which would make updates much easier but we need to check if Hydra still builds it then).

primeos added a commit to primeos/nixpkgs that referenced this issue Apr 26, 2021
The purpose of this package is to continuously improve the LLVM
packaging in Nixpkgs without causing a lot of rebuilds and provide more
recent LLVM builds for users. For more details see:
NixOS#114828
@primeos primeos linked a pull request Apr 26, 2021 that will close this issue
10 tasks
@primeos
Copy link
Member

primeos commented Apr 26, 2021

I've drafted a PR for llvmPackages_git based on the upstream Git monorepo instead of the individual tarballs: #120780

It's far from perfect and a clean slate approach would be nicer but so far it doesn't seem realistic that someone will put in the effort. Therefore it seems like a better idea to start by copying llvmPackages_12 and improve / clean it up incrementally (with much less impact on Nixpkgs as nothing depends on it).

@Ericson2314
Copy link
Member

Ericson2314 commented Apr 28, 2021

I absolutely agree we should have a derivation to track HEAD, but i think it should be built in the manner of the others.

Until we are using CA derivations (NixOS/rfcs#62) in Nixpkgs, we have no good way to filter stuff out of sources without polluting downstream hashes, so the current approach, as cumbersome as it is, is necessary to ensure individual packages can be patched whatnot nicely with minimal rebuilds.

(And to be clear, somehow filtering the source is a must because otherwise we could accidentally rebuild parts of old packages. The LLVM build system is pretty intense, and I absolutely don't trust it to ignore source code it shouldn't care about.)

I'm sorry if that's a bit of a bummer, it's precisely cause these things are annoying that I worked on NixOS/rfcs#62 in the first place!

@primeos
Copy link
Member

primeos commented Apr 28, 2021

@Ericson2314 no sure what you mean here - could you e.g. give an example?

I absolutely agree we should have a derivation to track HEAD, but i think it should be built in the manner of the others.

AFAIK that's not possible (the tarballs are release assets, so we'd have to generate them manually for non-releases - which is of course possible but I don't see how this would be a good idea).

we have no good way to filter stuff out of sources without polluting downstream hashes, so the current approach, as cumbersome as it is, is necessary to ensure individual packages can be patched whatnot nicely with minimal rebuilds.

Upstream switched to the mono repository layout so I don't see why we should continue using our hacks for combining individual tarballs to fix builds. IMO we should avoid patches as much as possible and I don't really see the rebuilds problem (we can still use the patch phase as usual).

I absolutely don't trust it to ignore source code it shouldn't care about.

Maybe that's the actual problem (i.e. that we wouldn't know which packages we need to patch)? With upstream transitioning more and more to the mono repository layout I don't see how this'll be easily avoidable.

Anyway, should I read this as a veto against #120780 or not?

@Ericson2314
Copy link
Member

Ericson2314 commented Apr 28, 2021

AFAIK that's not possible (the tarballs are release assets, so we'd have to generate them manually for non-releases - which is of course possible but I don't see how this would be a good idea).

It's possible to delete stuff at fetch time in

, extraPostFetch ? ""

but that's quiet annoying, and I can't think of a time we bumped the fetched sources individually.

So I relent, it's fine to fetch everything at once and then split up.

Upstream switched to the mono repository layout so I don't see why we should continue using our hacks for combining individual tarballs to fix builds.

We can do src = runCommand "${pname}-src}" {} "take just what we need from ${src}";

Maybe we should also apply patches in the source post-processing derivations too.

IMO we should avoid patches as much as possible and I don't really see the rebuilds problem (we can still use the patch phase as usual).

Yes, per project patching is good.

Maybe that's the actual problem (i.e. that we wouldn't know which packages we need to patch)? With upstream transitioning more and more to the mono repository layout I don't see how this'll be easily avoidable.

Upstream still aims to support separate builds. I think they CI for it too. If we whitelist the source subdirs as described above to replicate how individual tarballs were combined previously, it should be fine.

Anyway, should I read this as a veto against #120780 or not?

Well I think there's still good stuff in there even if you do change to this approach? I'm thinking let's land #111487 next and then redo #120780 in light of this and matching the general normalization I did in that. I'm happy to help out.

@primeos
Copy link
Member

primeos commented Apr 28, 2021

We can do src = runCommand "${pname}-src}" {} "take just what we need from ${src}";

That seems like a feasible option. No sure if it's really worth it but at least it would require few extra effort.

Upstream still aims to support separate builds.

I didn't have that impression so far (they seem to increasingly hard-code paths according to the mono repository layout) but maybe it'll get better?

If we whitelist the source subdirs as described above to replicate how individual tarballs were combined previously, it should be fine.

Yeah, that should remain fine.

Well I think there's still good stuff in there even if you do change to this approach?

Not sure - db1ce99 is the relevant diff and might indeed safe some time. I guess we should then replace src (and ideally also sourceRoot but I don't see an elegant solution without modifying stdenv.mkDerivation) with some function that takes a list of relevant subdirectories.

I'm thinking let's land #111487 next and then redo #120780 in light of this and matching the general normalization I did in that.

Sounds good.

I'm happy to help out.

Thanks! That would be useful as I'm currently low on time and don't have much motivation to try out different approaches.

Tbh I'm already thinking about removing myself as LLVM maintainer as I was hoping to heavily refactor/modernize (/ incrementally rewrite) llvmPackages_git but after our discussion in #120506 (comment) I'm no longer too optimistic that this'll really help as we still depend too much on old/ancient LLVM versions and I'm starting to realize that some PRs might indeed want/need to modify all of them (I was hoping that for most PRs it would be enough to fix only the most recent LLVM version but for cross-compiling, etc. this obviously isn't enough).

@Ericson2314
Copy link
Member

I didn't have that impression so far (they seem to increasingly hard-code paths according to the mono repository layout) but maybe it'll get better?

I still see lots of conditional "is this a separate project" code and the beginning of CMake files and whatnot. I do fear most of the devs mainly do everything builds (even before the monorepo), so there is a bit of a bias for that, but as long as the official policy isn't changing we're good.

Tbh I'm already thinking about removing myself as LLVM maintainer...

Sorry to hear that, but I do empathize. I would be nice if we could retire old versions after. I think Darwin is supposed to finally leave 7 behind, which should help with that? (It was stuck there a while.)

primeos added a commit to primeos/nixpkgs that referenced this issue Jun 3, 2021
The purpose of this package is to continuously improve the LLVM
packaging in Nixpkgs without causing a lot of rebuilds and provide more
recent LLVM builds for users. For more details see:
NixOS#114828
@attila-lendvai
Copy link
Contributor

this is somewhat offtopic here, sorry about that!

but is there a way to get llvmPackages_git binaries from hydra, without local recompilation?

this is what i'm doing in my default.nix for a project:

{}:

let

  # to use llvm from the unstable channel
  pkgs = import (fetchTarball https://github.com/NixOS/nixpkgs/archive/nixpkgs-unstable.tar.gz) {};
  #pkgs = import <nixpkgs> {};
  llvmPackages = pkgs.llvmPackages_git;

in

llvmPackages.clang.stdenv.mkDerivation {
  pname = "clasp";
  version = "unstable";

  src = ./.;

  nativeBuildInputs = with pkgs; [
    python git sbcl wafHook
    llvmPackages.llvm
    llvmPackages.clang
    libffi.dev boehmgc.dev
  ];

  buildInputs = with pkgs; [
    llvmPackages.libclang.out
    libffi boehmgc
    gmp zlib ncurses
    libelf libbsd boost
  ];

  buildTargets = "build_cboehm";
  installTargets = "install_cboehm";

  shellHook =
  ''
    alias ..='cd ..'
    alias ...='cd ../..'
  '';
}

@primeos
Copy link
Member

primeos commented Jun 7, 2021

Indeed, aliases aren't built on Hydra and there doesn't seem to be an existing way to bypass that in aliases.nix:

# Removing recurseForDerivation prevents derivations of aliased attribute
# set to appear while listing all the packages available.
removeRecurseForDerivations = alias: with lib;
if alias.recurseForDerivations or false then
removeAttrs alias ["recurseForDerivations"]
else alias;
# Disabling distribution prevents top-level aliases for non-recursed package
# sets from building on Hydra.
removeDistribute = alias: with lib;
if isDerivation alias then
dontDistribute alias
else alias;

This may or may not be desirable but in any case llvmPackages_git isn't useful yet and updates require quite some effort ATM: #125698 (and I lack motivation to deal with some of those issues).

@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
Labels
0.kind: packaging request Request for a new package to be added 6.topic: llvm/clang Issues related to llvmPackages, clangStdenv and related
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants