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

Parse comments to build bottles for new distros #556

Merged
merged 7 commits into from
Nov 30, 2021

Conversation

scpeters
Copy link
Contributor

@scpeters scpeters commented Nov 13, 2021

Comment "build bottle --keep-old" to build bottles for new macOS versions only if the homebrew-simulation pull request doesn't change the package version. This is a partial fix for #555 that doesn't allow specifying a single version of macOS. As such it only works for packages that have no current bottle for any currently supported version (such as simbody and ignition-math4) at this time.

Update: you can now comment build bottle --keep-old --only-big_sur to build new bottles for Big Sur. This allows us to keep existing bottles while adding bottles for newly supported distributions. Any macOS version is supported (i.e. --only-catalina) using the distro name used in the bottle blocks.

Comment "build bottle --keep-old" to build bottles
for new macOS versions only if the homebrew-simulation
pull request doesn't change the package version.

Signed-off-by: Steve Peters <[email protected]>
@scpeters scpeters force-pushed the scpeters/brew_keep_old branch from f5ea749 to 94a00c0 Compare November 13, 2021 02:35
@scpeters
Copy link
Contributor Author

After initial testing with ignition-mat4 osrf/homebrew-simulation#1675, I realized we need to use the --keep-old flag in the brew bottle command in the bottle hash pr updater job, which I added in 0e383ab. I managed a successful test with simbody in osrf/homebrew-simulation#1676 and think this is ready to merge

@scpeters
Copy link
Contributor Author

This is a partial fix for #555 that doesn't allow specifying a single version of macOS.

I managed to implement support for specifying which macOS versions to build for in 50142ab. I can add it to this pull request or make a separate one after this is merged.

@scpeters
Copy link
Contributor Author

scpeters commented Nov 15, 2021

along with merging this, we should update the documentation at https://github.com/osrf/homebrew-simulation/blob/master/README.md

@j-rivero
Copy link
Contributor

Comment "build bottle --keep-old" to build bottles for new macOS versions only if the homebrew-simulation pull request doesn't change the package version. This is a partial fix for #555 that doesn't allow specifying a single version of macOS. As such it only works for packages that have no current bottle for any currently supported version (such as simbody and ignition-math4) at this time.

This increases a little bit the management of building bottles at our end since the person building them should now about the flags and when to use them. I was wondering if we somehow we can infer the use of the flags from changes in PR, code in formula or other methods (or even run the flags unconditionally) or the human factor to decide is crucial to employ them.

@scpeters
Copy link
Contributor Author

This increases a little bit the management of building bottles at our end since the person building them should now about the flags and when to use them. I was wondering if we somehow we can infer the use of the flags from changes in PR, code in formula or other methods (or even run the flags unconditionally) or the human factor to decide is crucial to employ them.

most of the time, people should just use build bottle. Only when we add support for a new version of macOS do we need to use --keep-old. In homebrew-core, they have a special GitHub action with arguments for a formula name and the macOS version to build for, and it will build the bottle and possibly commit it directly to master. It was expedient to modify use our existing job to support this behavior. The requirement to use --keep-old is that the formula version does not change; so I've made only cosmetic changes in the pull requests using this feature like osrf/homebrew-simulation#1676. The cosmetic changes are necessary to indicate which formula should be built

@scpeters
Copy link
Contributor Author

This is a partial fix for #555 that doesn't allow specifying a single version of macOS.

I managed to implement support for specifying which macOS versions to build for in 50142ab. I can add it to this pull request or make a separate one after this is merged.

I just merged 50142ab into this branch

@scpeters scpeters changed the title Parse --keep-old from brew PR comments Parse comments to build bottles for new distros Nov 16, 2021
@scpeters
Copy link
Contributor Author

This is a partial fix for #555 that doesn't allow specifying a single version of macOS.

I managed to implement support for specifying which macOS versions to build for in 50142ab. I can add it to this pull request or make a separate one after this is merged.

I just merged 50142ab into this branch

and I tested it with osrf/homebrew-simulation#1677

@j-rivero
Copy link
Contributor

most of the time, people should just use build bottle. Only when we add support for a new version of macOS do we need to use --keep-old. In homebrew-core, they have a special GitHub action with arguments for a formula name and the macOS version to build for, and it will build the bottle and possibly commit it directly to master. It was expedient to modify use our existing job to support this behavior. The requirement to use --keep-old is that the formula version does not change; so I've made only cosmetic changes in the pull requests using this feature like osrf/homebrew-simulation#1676. The cosmetic changes are necessary to indicate which formula should be built

Thanks for the details, help to understand the new use case. It seems to me that we are trying to support the build of bottles in new platforms, which requires to use the --keep-old parameter in combination with cosmetic changes to the formula we want to build. I see at least to problems:

  • Use the name of --keep-old for triggering a process of creating new bottles for a new distribution is not intuitive for those of us without knowledge of Brew. We could use something like --build-for-new-distro or similar can help, we can translate that to --keep-old in the scripts but seems easy to understand what's going on in the PR.
  • Having to make cosmetic changes to the formulas is somehow a bit ugly. I understand that is how build-bot works and probably hard to workaround. Bumping revision number could be an alternative solution, we are doing that in Debian when we need to move a package from one distribution (experimental) to another (unstable).

This reverts commit 2120c48.

Signed-off-by: Steve Peters <[email protected]>
@scpeters
Copy link
Contributor Author

  • Use the name of --keep-old for triggering a process of creating new bottles for a new distribution is not intuitive for those of us without knowledge of Brew. We could use something like --build-for-new-distro or similar can help, we can translate that to --keep-old in the scripts but seems easy to understand what's going on in the PR.

we could also combine the flags like --build-for-new-distro-big_sur or make is a phrase instead of something that looks like a CLI parameter. It just needs to be parseable by a regex so we have flexibility on the technical side to make it more humane

  • Having to make cosmetic changes to the formulas is somehow a bit ugly. I understand that is how build-bot works and probably hard to workaround. Bumping revision number could be an alternative solution, we are doing that in Debian when we need to move a package from one distribution (experimental) to another (unstable).

The better solution than cosmetic changes to signal which formula to build would be to specify the formula name as a parameter to the jenkins job. This would require a new jenkins job and we'd probably have to trigger it directly from build.osrfoundation.org, then it would run the build and open a pull request with the pr-bottle-hash-update. I didn't do this at first since it was easier for me to abuse the existing job than figure out how to create a new one

@scpeters
Copy link
Contributor Author

* Bumping revision number could be an alternative solution

we lose old bottles when we bump the revision number. I don't know if there are still people using bottles for old macOS versions, but it seems a shame to invalidate them if it's not necessary. Also, it saves some CI time to keep the catalina bottle if it's already there if we only need a new big_sur bottle.

@scpeters
Copy link
Contributor Author

what if I just automatically set --keep-old if I detect a --only-* option?

@j-rivero
Copy link
Contributor

what if I just automatically set --keep-old if I detect a --only-* option?

sounds like a good idea to me.

we lose old bottles when we bump the revision number. I don't know if there are still people using bottles for old macOS versions, but it seems a shame to invalidate them if it's not necessary. Also, it saves some CI time to keep the catalina bottle if it's already there if we only need a new big_sur bottle.

+1

How about if we use something different than the cli parameter syntax to send command to brew bot, something like: brew-bot-tag: build-for-new-distro-big_sur

@scpeters
Copy link
Contributor Author

we lose old bottles when we bump the revision number. I don't know if there are still people using bottles for old macOS versions, but it seems a shame to invalidate them if it's not necessary. Also, it saves some CI time to keep the catalina bottle if it's already there if we only need a new big_sur bottle.

+1

How about if we use something different than the cli parameter syntax to send command to brew bot, something like: brew-bot-tag: build-for-new-distro-big_sur

I'm flexible on the syntax; I just want it to be not too complex to parse. My idea with CLI arguments was to avoid whitespace complexities, but I think it shouldn't be too hard to parse brew-bot-tag: build-for-new-distro-big_sur

I'll give it a try

Use brew-bot-tag: build-for-new-distro-
instead of --keep-only and --only-

Signed-off-by: Steve Peters <[email protected]>
@scpeters
Copy link
Contributor Author

How about if we use something different than the cli parameter syntax to send command to brew bot, something like: brew-bot-tag: build-for-new-distro-big_sur

I'm flexible on the syntax; I just want it to be not too complex to parse. My idea with CLI arguments was to avoid whitespace complexities, but I think it shouldn't be too hard to parse brew-bot-tag: build-for-new-distro-big_sur

I'll give it a try

36a1e7c

I'll open a new homebrew-simulation pull request to test it

@scpeters
Copy link
Contributor Author

testing with osrf/homebrew-simulation#1694 after deplying the DSL from this branch and reconfiguring the bottle builder job to use this branch

@scpeters
Copy link
Contributor Author

testing with osrf/homebrew-simulation#1694 after deplying the DSL from this branch and reconfiguring the bottle builder job to use this branch

Build Status https://build.osrfoundation.org/job/generic-release-homebrew_triggered_bottle_builder/686/

@scpeters
Copy link
Contributor Author

I've added documentation for the new syntax in osrf/homebrew-simulation#1695

Copy link
Contributor

@j-rivero j-rivero left a comment

Choose a reason for hiding this comment

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

good to go, IMHO. Thanks for iterating Steve, cool stuff.

@scpeters scpeters merged commit 205fce5 into master Nov 30, 2021
@scpeters scpeters deleted the scpeters/brew_keep_old branch November 30, 2021 22:33
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