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

Fix missing git dependencies when building monorepos #1161

Merged
merged 4 commits into from
Jan 15, 2024

Conversation

f-f
Copy link
Member

@f-f f-f commented Jan 12, 2024

Our build at work uncovered a bug that seems to trigger only in very specific condition.

This PR contains:

  • a test that replicates the bug (and the first commit here shows that CI breaks on it)
  • the fix for said bug (that makes the test suite happy again)

I suspect the bug was likely introduced as a side effect of #1018 and exposed by the work in #1138, and it's due to improper glob handling during the build: we are constructing the globs for the whole build when querying the graph, and reusing the same globs for the build. However, since #1138 we do not fetch all the dependencies prior to building, so when building a single package in a monorepo, we could stumble on dependencies that have not been fetched yet (because the fetch step will only fetch the dependencies that we need at the moment).
This could in principle happen with any kind of dependency that we fetch from the internet, but doesn't reliably happen with Registry packages, because we heavily cache them. Fetching a git repo provides a reliable repro instead.

The fix in this patch prepares two sets of globs, one for purs graph and one for purs compile. This seems to fix things, but I'm not entirely convinced it's the most sensible thing we could do. Maybe we should always fetch all dependencies instead, even when building a single package in a monorepo? Something to ponder, I think the current fix works fine for now.

@JordanMartinez
Copy link
Contributor

Since this PR uses two different globs in purs compile vs purs graph, what happens when --strict is used in this combination? Will that potentially throw an error as well because the purs graph globs reference irrelevant files?

It's been a while since I've looked at this code, so I don't recall the interaction between these two globs.

@f-f
Copy link
Member Author

f-f commented Jan 12, 2024

Yes indeed - I just stumbled on this but with --pedantic-packages instead of --strict. I'll put together a new test and look for a fix

@f-f
Copy link
Member Author

f-f commented Jan 15, 2024

@JordanMartinez I got rid of the different set of globs, and just restricted to the package globs if we are building with a single package. I also updated the test accordingly, so this should be good for another look

@f-f f-f merged commit 6f9163f into master Jan 15, 2024
3 checks passed
@f-f f-f deleted the fix-missing-dependencies branch March 8, 2024 09:57
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