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 git repo URL to fix carapace build #218

Merged
merged 1 commit into from
Jan 29, 2025
Merged

Conversation

kylewlacy
Copy link
Member

Followup to #214, it seems like the git repo URL https://github.com/carapace-sh/carapace.git was used, where it should be https://github.com/carapace-sh/carapace-bin.git. I missed that during the review!

Verified locally that the build works with the adjusted repo URL

@kylewlacy kylewlacy enabled auto-merge (squash) January 29, 2025 10:04
@jaudiger
Copy link
Contributor

I missed that too, sorry !

@kylewlacy kylewlacy merged commit 0fe2f5a into main Jan 29, 2025
2 checks passed
@kylewlacy kylewlacy deleted the fix-carapace-build-error branch January 29, 2025 10:10
@jaudiger
Copy link
Contributor

jaudiger commented Jan 29, 2025

Did you detect it with the CI ?

Edit: Found it: https://github.com/brioche-dev/brioche-packages/actions/runs/13025160105/job/36333743770. Do you think we should then run the build step inside the PR to prevent this to happen again ?

@kylewlacy
Copy link
Member Author

Do you think we should then run the build step inside the PR to prevent this to happen again ?

Hmm, I'm honestly not sure...

Builds are really slow, and they get run on a single self-hosted server, so I've tried to be a bit selective on when builds run. The concurrency feature on GitHub Actions has worked really well for us, where there can only ever be one build running at a time. If a PR gets merged while a build is running, it'll wait until the first build finishes. If a second PR gets merged too, then it'll replace the previous waiting build so there's only ever one waiting build (in other words, the first merged commit will build, and the last merged commit will cancel any other waiting commits then wait until it can build).

We'd also have to decide how the cache should work for builds for PRs. If we don't cache the results, then we'd end up doing one "test build" in the PR, only to throw it away and do a "real build" once its merged. If we do cache the results, then any time the PR changes, we'd be storing the previous versions in the build cache too (at least until we add a feature to clean the build cache).

For my personal use, I have a rule set up so it triggers a build if the branch name starts with build/. So for bigger scale changes, I'll name my branch build/something, push it, and check the CI in the morning to see if the build succeeded. Also, these builds get cached, so if I merge the code without any other changes, I can merge to main and it won't need to rebuild again.

For now, I'm happy with the current flow. But I think we'll want to add a way to test bigger changes. Something like... a way for the reviewer to manually trigger the build for a PR that looks "risky". Maybe along with PR step that detects how many packages that need to be rebuilt to help decide that.


Also, once we get further with the autoUpdate feature, I do think we'll want to have automated PRs that trigger builds, since those PRs are commonly going to be merged without any manual changes, and those will probably make up a good chunk of all PRs.

@kylewlacy
Copy link
Member Author

...but in the future, if we have more GitHub Actions runners, or faster GitHub Actions runners, or a sponsor helping with costs, etc. then it might make sense to fully automate all PRs. Definitely worth trying to figure out options here since automated checks are super valuable

@jaudiger
Copy link
Contributor

Ohh okay. I wasn't aware of the trick with prepending the branch with build/. I'll do it next time, I'm updating too many packages in parallel in the same PR.

I see your point. When you say the server is self-hosted ? Does it mean you run it at home, or is hosted somewhere on a cloud provider infrastructure on a small instance ? And what is the current spec of the instance ? Maybe we could discuss that somewhere rather than in this PR 😆

Well, I was asking for the build inside PR because since I'm using an ARM machine (Apple MacBook Pro M1), I need to virtualize brioche inside a docker image running with Rosetta (that translates instructions between x86 and ARM architectures). It's not that slow, but I can see the difference when building something inside this virtual environment. Thus, testing all the packages by building them can sometimes take time. But I'm trying to do as much as possible. Well, for this PR, I was doing it but on random packages. So, my fault for the faulty carapace URL and for jq building.

I see what you mean, it reminds me of the Nixpkgs repository where they have a specific job to compute the labels of a PR (https://github.com/NixOS/nixpkgs/actions/runs/13040123824/job/36379874774?pr=377814). These labels inform for example on the number of packages impacted by the modification in the PR, and so on. I understand right now that we can't run build on the global directory like that. But, maybe, we could do something like that:

  • Run the build on the global directory on main branch
  • Run the build on the brioche projects affected by the files modified. It will still be approximate, it won't take into account the dependencies injection. But at least, it's a step in better testing the PR before merging them.

@kylewlacy
Copy link
Member Author

(For visibility, I replied to this conversation through Discord: https://discord.com/channels/1246413282894413844/1246601881585975441/1334409958808227880)

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