-
-
Notifications
You must be signed in to change notification settings - Fork 14.6k
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
llvmPackages.libc: init at 20.0.0-unstable-2024-12-08 #363449
base: master
Are you sure you want to change the base?
Conversation
91c39aa
to
44e5254
Compare
44e5254
to
c16e82d
Compare
c16e82d
to
50de490
Compare
The non-full variant doesn't seem to be used? |
As of this time, the overlay build isn't a dependency for anything. |
What is "the overlay build"? I don't see it explained anywhere here. |
The overlay build is the non full build of LLVM libc. |
What's the point of it? |
I asked the LLVM libc team on their Discord server:
|
What does "kinda borked" mean? |
The full build has some problems still needing to be worked out. There's some odd C++ errors preventing compiling. Overlay builds work. |
So then why add two attributes, one that works and one that doesn't? Why not just one libc attribute that builds as much as we can? Is this actually useful for anything yet? If not, maybe we should just wait until we can at least build it properly. |
I was recommended to have both as an option since it'll allow for flexibility if things are broken. I'm going to be working on fixing the full build soon, just haven't gotten time. I've got some PR's merged upstream and some other might've flown under my radar so it could be working. |
For flexibility, wouldn't a single attribute with an option we could flip be better? Otherwise, we'd have to change everything that referred to the attribute all at once if we wanted to go from full to overlay. |
My thinking was to probably have 3, two are the actual builds and one is an alias. I think other parts of LLVM does this already in nixpkgs. |
But if the full build is working, why would we ever need the alias? Why does it need an attribute too? |
We already have to set a boolean anyways for full builds. The dependencies are different too. Providing both makes it easier to test if both work and for people to experiment with. |
If full is building, why would I want to test with overlay? |
I was meaning people who write software using nix could try it out. I know some people who would want to try it out. |
Yes, but is there a reason they'd specifically want to test out the non-full version, even if full is available and building? |
Got it. In that case, we should probably support both in libcCrossChooser, so people can easily switch between them, right? The reason this looked wrong to me at the start was that there was no way to really use one of them. |
We'd need some sort of stub libc in the overlay one to pull in both since the overlay uses a different library name. |
Oh, it's not called libc? That's weird — how is it supposed to be used then? |
|
With -nostdlib I guess? |
No, you need a libc to overlay on top of. So you'd have both. |
); | ||
}; | ||
} | ||
libc = if stdenv.targetPlatform.libc == "llvm" then libraries.libc-full else libraries.libc-overlay; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this attribute being for two different things depending on the platform is going to be confusing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tbf, we already have attributes which confuse people. clangUseLLVM
, compiler-rt
, stdenv
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And that's justification to make things more confusing??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well I think it's not that confusing, it'll cause some accidents until someone realizes which one they should use if they need it. The lib names are different between the two so someone could easily tell when things go wrong when an error happens. Also, documenting the attributes better would help a lot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So why not make it explicit whether full or overlay is being used? As we've established, they're used completely differently, so there isn't a situation where you'd want an attribute that was sometimes the overlay and sometimes full.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My thinking was having a shim which adds in both inside the overlay mode. Then the function which selects the libc cross could detect when to use the overlay mode. That could allow for mixing LLVM libc and glibc or LLVM libc and musl.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I know, a shim can be made by writing a linker script into libc.so
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then how about we hold off on this attribute until that's ready to go? It's not really useful as it is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added the shim.
5fbd87e
to
d83fcfa
Compare
d83fcfa
to
41bcc2c
Compare
41bcc2c
to
e2a5faf
Compare
e2a5faf
to
f72b341
Compare
ec43413
to
f1645ab
Compare
@@ -91,7 +91,7 @@ let | |||
&& final.parsed.kernel == platform.parsed.kernel; | |||
isCompatible = _: throw "2022-05-23: isCompatible has been removed in favor of canExecute, refer to the 22.11 changelog for details"; | |||
# Derived meta-data | |||
useLLVM = final.isFreeBSD || final.isOpenBSD; | |||
useLLVM = final.isFreeBSD || final.isOpenBSD || final.isLLVMLibc; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OOC does it make sense to ever use LLVM libc with GCC?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hard to say based on whether GCC supports the naked attribute for specific targets. I was thinking the LLVM abi which means to use the libc that it'd make sense to use the entire toolchain. That's what the team upstream thinks as well.
Things done
Adds LLVM libc, we've got the overlay building but full is still kinda borked. We're only going to support from >= LLVM 20 because it requires a lot of patching in previous versions. We're hoping the inclusion of LLVM libc at the point it is right now could help further its development and we can figure out how to package it better early on.
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 👍 reaction to pull requests you find important.