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

Nix file sets, attempt 2.... #6146

Merged
merged 3 commits into from
Dec 29, 2023

Conversation

JRMurr
Copy link
Contributor

@JRMurr JRMurr commented Dec 2, 2023

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

@JRMurr JRMurr force-pushed the revert-6136-revert-nix-changes branch from bfa5ce4 to 536e97d Compare December 2, 2023 19:38
Comment on lines 3 to 5
#[cfg(test)]
mod tests;

Copy link
Contributor Author

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.

@Anton-4
Copy link
Collaborator

Anton-4 commented Dec 5, 2023

Feel free to @ me or ask on zulip if you want CI to run @JRMurr

@JRMurr
Copy link
Contributor Author

JRMurr commented Dec 5, 2023

possible the issue is related to LTO in clang? seems weird that this would pop up now NixOS/nixpkgs#19098

Will investigate

@Anton-4
Copy link
Collaborator

Anton-4 commented Dec 6, 2023

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.

@JRMurr JRMurr force-pushed the revert-6136-revert-nix-changes branch from 536e97d to 2172477 Compare December 12, 2023 01:46
@JRMurr
Copy link
Contributor Author

JRMurr commented Dec 12, 2023

@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....)

@JRMurr
Copy link
Contributor Author

JRMurr commented Dec 12, 2023

If this doesnt work we can try setting the rust flags to something like

-C link-args=-fno-lto

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

@JRMurr
Copy link
Contributor Author

JRMurr commented Dec 12, 2023

o looks like it already was using lld...

"-nodefaultlibs" "-fuse-ld=lld" "-fuse-ld=lld"

@JRMurr JRMurr force-pushed the revert-6136-revert-nix-changes branch from 2172477 to 5d04862 Compare December 14, 2023 23:35
@JRMurr
Copy link
Contributor Author

JRMurr commented Dec 14, 2023

@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.

@Anton-4
Copy link
Collaborator

Anton-4 commented Dec 15, 2023

Thanks for trying! I'll investigate a bit, we'll have to upgrade nixpkgs some day 😄

@Anton-4 Anton-4 self-assigned this Dec 15, 2023
@JRMurr
Copy link
Contributor Author

JRMurr commented Dec 15, 2023

looks like my other branch caused merge conflicts, will rebase tonight

@JRMurr JRMurr force-pushed the revert-6136-revert-nix-changes branch from 5d04862 to 824b08d Compare December 15, 2023 22:48
@JRMurr
Copy link
Contributor Author

JRMurr commented Dec 15, 2023

@Anton-4 should be good for review

@Anton-4
Copy link
Collaborator

Anton-4 commented Dec 16, 2023

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.

@Anton-4
Copy link
Collaborator

Anton-4 commented Dec 23, 2023

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.

@Anton-4
Copy link
Collaborator

Anton-4 commented Dec 26, 2023

Once #6314 is merged we can go back to the original approach here :)

@JRMurr
Copy link
Contributor Author

JRMurr commented Dec 27, 2023

@Anton-4 TY for figuring out the issue! Will update this pr with the og approach!

@JRMurr JRMurr force-pushed the revert-6136-revert-nix-changes branch from 824b08d to 316bd0d Compare December 27, 2023 17:55
@JRMurr
Copy link
Contributor Author

JRMurr commented Dec 27, 2023

@Anton-4 did a rebase and went back to the og approach. should be good for review

Copy link
Collaborator

@Anton-4 Anton-4 left a 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 :)

@Anton-4 Anton-4 merged commit a62c0ce into roc-lang:main Dec 29, 2023
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants