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

Use nix file set API to reduce how often we will need to rebuild #6113

Merged
merged 7 commits into from
Dec 1, 2023

Conversation

JRMurr
Copy link
Contributor

@JRMurr JRMurr commented Nov 29, 2023

some context https://roc.zulipchat.com/#narrow/stream/316715-contributing/topic/refactor.20nix.20files.2Fdefault.20build/near/404518156

What this is

This uses the new file set API (links at the bottom) for nix to reduce how many files we pass as the source to the nix builder for roc.

The bulk of the logic is is nix/fileFilter.nix, I added comments above each filter to call out what it does but please let me know if I should explain more.

TLDR of whats filtered

  • all folders named tests - we don't currently run tests in the nix build so they were not needed
  • only files in crates folder + a few files in the repo root like Cargo.{toml, lock}
  • removes all .md, .svg, and .png
  • removes www folder from checkmate

We can probably do more aggressive filtering especially if we refactor where we put the test only crates but I think this is a decent balance for now so if new crates/files are added people don't get weird errors they would not expect.

Misc changes

I had to update nixpkgs in the flake to get the new apis. I didnt notice any issues locally but might need someone to sanity check me

some links

@Anton-4 Anton-4 self-assigned this Nov 29, 2023
Signed-off-by: Anton-4 <[email protected]>
@Anton-4
Copy link
Collaborator

Anton-4 commented Nov 29, 2023

Thanks for working on this @JRMurr :)
I noticed a couple of TODO's, is this ready for review?

@JRMurr
Copy link
Contributor Author

JRMurr commented Nov 30, 2023

Thanks for working on this @JRMurr :) I noticed a couple of TODO's, is this ready for review?

Ahh my bad, will remove those. Good to review otherwise

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 @JRMurr!

@Anton-4 Anton-4 merged commit f7841f0 into roc-lang:main Dec 1, 2023
11 of 15 checks passed
Anton-4 added a commit that referenced this pull request Dec 1, 2023
Anton-4 added a commit that referenced this pull request Dec 1, 2023
This reverts commit f7841f0, reversing
changes made to b4506a4.
rtfeldman added a commit that referenced this pull request Dec 1, 2023
Revert "Merge pull request #6113 from JRMurr/nix-build-file-sets"
@Anton-4
Copy link
Collaborator

Anton-4 commented Dec 1, 2023

I've reverted this PR @JRMurr, it sneaked through our CI, not yet sure how. It was hitting this issue, I'm not yet sure what exactly caused it.

@JRMurr
Copy link
Contributor Author

JRMurr commented Dec 1, 2023

@Anton-4 ahh my bad

thats a strange error, "in theory" my changes should only affect the nix build but not the dev env. Ill see if i can find a repro

@JRMurr
Copy link
Contributor Author

JRMurr commented Dec 1, 2023

Ahh it was probably the flake update actually

@Anton-4
Copy link
Collaborator

Anton-4 commented Dec 1, 2023

@Anton-4 ahh my bad

Not your fault, CI should have caught it

@Anton-4
Copy link
Collaborator

Anton-4 commented Dec 1, 2023

Ahh it was probably the flake update actually

That would be my guess as well

JRMurr added a commit to JRMurr/roc that referenced this pull request Dec 2, 2023
JRMurr added a commit to JRMurr/roc that referenced this pull request Dec 2, 2023
JRMurr added a commit to JRMurr/roc that referenced this pull request Dec 12, 2023
JRMurr added a commit to JRMurr/roc that referenced this pull request Dec 14, 2023
JRMurr added a commit to JRMurr/roc that referenced this pull request Dec 15, 2023
JRMurr added a commit to JRMurr/roc that referenced this pull request Dec 27, 2023
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