-
-
Notifications
You must be signed in to change notification settings - Fork 321
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
Nix file sets, attempt 2.... #6146
Conversation
bfa5ce4
to
536e97d
Compare
crates/wasm_interp/src/lib.rs
Outdated
#[cfg(test)] | ||
mod tests; | ||
|
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 originally removed this mod tests
since the folder wouldn't exist during the nix build and it is not need outside of tests.
This caused
impl<'a, I: ImportDispatcher> Instance<'a, I> {
#[cfg(test)]
pub(crate) fn new<G>(
to have clippy errors since it was only used in the tests.
Feel free to @ me or ask on zulip if you want CI to run @JRMurr |
possible the issue is related to LTO in clang? seems weird that this would pop up now NixOS/nixpkgs#19098 Will investigate |
Let me know if it's difficult to make progress. I can take a look at this then. I imagine it's quite hard without an apple silicon device. |
536e97d
to
2172477
Compare
@Anton-4 ty for the offer! I updated the nix shell to use lld since we have it in the nix env. Hopefully this works around the issue + for everyone using nix (including linux) should get faster builds automagically (assuming this actually works....) |
If this doesnt work we can try setting the rust flags to something like
for just arm mac but that feels like it might be pretty bad perf wise. If it comes to that ill look more deeply into trying to pin the clang linker to the version on main |
o looks like it already was using lld...
|
2172477
to
5d04862
Compare
@Anton-4 I threw in the towel a bit... I changed it so we only use the unstable input for the flile filtering api and not for any "real" packages. |
Thanks for trying! I'll investigate a bit, we'll have to upgrade nixpkgs some day 😄 |
looks like my other branch caused merge conflicts, will rebase tonight |
5d04862
to
824b08d
Compare
@Anton-4 should be good for review |
Thanks @JRMurr, in the coming days I'll try to see if I can fix the earlier issue on macos arm64 and will merge if not. |
After a long and difficult bisect journey I was able to find the nix commit that caused the issue. Follow up discussion will happen in #6303. |
Once #6314 is merged we can go back to the original approach here :) |
@Anton-4 TY for figuring out the issue! Will update this pr with the og approach! |
824b08d
to
316bd0d
Compare
@Anton-4 did a rebase and went back to the og approach. should be good for review |
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.
Thanks for the sustained effort @JRMurr :)
Some ci errors slipped in from #6113 so it was reverted
My guess is the flake update I did. I attempted flake update but this time also updated the rust overlay. My hope is the rust overlay update might better be tracking nix unstable so its ld is also updated.
Shot in the dark since i dont have an m1 :(
If CI fails again, it would be possible to add a "new" nixpkgs input to the flake and use that just for the lib api and not for its pkgs