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

Remove LTO-only builds when PGO+LTO exists #223

Merged
merged 1 commit into from
Mar 9, 2024

Conversation

charliermarsh
Copy link
Collaborator

Summary

This PR removes lto builds for cases in which pgo+lto exists, which looked like a good first task to familiarize myself with the project structure.

Closes #220.

@charliermarsh
Copy link
Collaborator Author

Unclear on the best way to test this (if any?). My methodology was to grep for pgo+lto, then work backwards to identify the sites at which lto should be removed.

Copy link
Owner

@indygreg indygreg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The important change here is to release.rs, which selects which builds to release. Strictly speaking we could continue to build the LTO only configurations and just not release them. There are plenty of build configurations we build but don't release.

But, this project already suffers from too many CI jobs. More jobs, more chance of failure. I really don't think the LTO only builds provide much value on their own. So I'm fine removing them from CI as well.

As for the CI failure, could you please try rebasing onto latest main commit? I think I fixed a bug in CI from forks.

@indygreg
Copy link
Owner

indygreg commented Mar 3, 2024

Actually, I cherry-picked the hopeful CI fix commit to your branch. Let's see if CI works now.

Feel free to rebase the branch at your leisure. The latest HEAD commit should disappear.

@charliermarsh
Copy link
Collaborator Author

@indygreg -- Thanks for cherry-picking. Want me to rebase or will you squash-merge either way?

@indygreg
Copy link
Owner

indygreg commented Mar 4, 2024

@indygreg -- Thanks for cherry-picking. Want me to rebase or will you squash-merge either way?

Please rebase. (I could do this myself but I try not to force push to other people's branches as a matter of courtesy since Git doesn't handle force pushes very well!)

I also try to have a linear, merge-free history in the repo. Up to this point usually I've been cherry picking commits locally and amending commit messages in flight. It isn't very GitHub-y. But I don't need to be a stickler for this preference going forward.

Copy link
Owner

@indygreg indygreg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@indygreg indygreg merged commit df9dce6 into indygreg:main Mar 9, 2024
8 of 247 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.

Stop releasing LTO only .tar.zst assets when PGO+LTO exists
2 participants