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

migrate sdl to sdl12-compat #111081

Closed
wants to merge 37 commits into from
Closed

migrate sdl to sdl12-compat #111081

wants to merge 37 commits into from

Conversation

cho-m
Copy link
Member

@cho-m cho-m commented Sep 19, 2022

  • Have you followed the guidelines for contributing?
  • Have you ensured that your commits follow the commit style guide?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing brew install --build-from-source <formula>)? If this is a new formula, does it pass brew audit --new <formula>?

Probably need to plan modification of conflicts with between sdl and sdl12-compat to avoid issues when upgrading.

@cho-m cho-m added do not merge in progress Stale bot should stay away labels Sep 19, 2022
@cho-m cho-m mentioned this pull request Sep 19, 2022
22 tasks
@carlocab
Copy link
Member

Probably need to plan modification of conflicts with between sdl and sdl12-compat to avoid issues when upgrading.

  1. Make sdl keg-only
  2. Add link_overwrite to sdl12-compat

@cho-m
Copy link
Member Author

cho-m commented Sep 19, 2022

A couple failures since sdl-config uses its own prefix in sdl12-compat while the HOMEBREW_PREFIX is used by sdl, i.e.

$ /usr/local/opt/sdl/bin/sdl-config --cflags
-I/usr/local/include/SDL -D_GNU_SOURCE=1 -D_THREAD_SAFE

$ /usr/local/opt/sdl12-compat/bin/sdl-config --cflags
-I/usr/local/Cellar/sdl12-compat/1.2.56/include/SDL -D_THREAD_SAFE

EDIT: This may mean sdl won't behave correctly if made keg-only.

Comment on lines -74 to -77
# we have to do this because most build scripts assume that all sdl modules
# are installed to the same prefix. Consequently SDL stuff cannot be
# keg-only but I doubt that will be needed.
inreplace %w[sdl.pc.in sdl-config.in], "@prefix@", HOMEBREW_PREFIX
Copy link
Member Author

Choose a reason for hiding this comment

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

This may break some users' usage of sdl, but changing formula to keg-only means using HOMEBREW_PREFIX would be broken without a manual brew link --force. Anyway, formula can be disable'd right after this PR.

Copy link
Member

@Bo98 Bo98 Sep 20, 2022

Choose a reason for hiding this comment

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

We could just fully get rid of sdl formula and merge it into sdl12-compat.

That was actually my initial ambition but I was waiting for a release to come out with bug fixes, which it now has.

This would bypass the whole 3 month deprecation period and a scenario where brew install sdl is a bit broken from being keg-only. Renaming should work nicely given it should be an almost identical install tree. Even the version number is specially crafted to appear like a SDL 1.2 patch update.

Copy link
Member Author

Choose a reason for hiding this comment

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

So delete sdl.rb and add "sdl": "sdl12-compat" to renames?

Copy link
Member

@Bo98 Bo98 Sep 20, 2022

Choose a reason for hiding this comment

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

I think so, yeah. Might be worth comparing the install trees first and addressing any differences.

Copy link
Member Author

@cho-m cho-m Sep 20, 2022

Choose a reason for hiding this comment

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

Ignoring man pages and some libexec files in sdl, the main differences are:

  • we no longer have static library libSDL.a (given that sdl12-compat only supports building this on Linux, don't have any choice here)
  • on Linux, fully versioned dynamic library is different
    • sdl uses libSDL-1.2.so.0.11.4 vs sdl12-compat uses libSDL-1.2.so.1.2.56
    • latter is a bit odd since libSDL-1.2.so.0 is symlinked to it, but this allows drop-in as long as package links to this name.
    • but could be seen as "upgrade" and users should rebuild with newer library for broken linkage if they didn't link to lib/libSDL-1.2.so.0.

Copy link
Member Author

@cho-m cho-m Sep 20, 2022

Choose a reason for hiding this comment

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

Probably need a syntax-only PR due to audit.

May need to look into dependents on whether we can avoid revision bumps.

Copy link
Member

@Bo98 Bo98 Sep 20, 2022

Choose a reason for hiding this comment

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

  • but could be seen as "upgrade" and users should rebuild with newer library for broken linkage if they didn't link to lib/libSDL-1.2.so.0.

If you are linking to a fully versioned libraries you're signing up for regular rebuilds (no one, for example, should be linking to libstdc++.so.6.0.30). This isn't anything to be concerned about.

The lack of a static library is odd, but we've shipped bigger breaking changes so should be fine.

Probably need a syntax-only PR due to audit.

For the two disabled formulae? Yeah that's probably fine.

I'd be cautious about everything else. Will require testing out how everything is currently linked to SDL. In theory though, yes it is designed to work as a drop-in replacement without needing a rebuild.

@cho-m cho-m removed do not merge in progress Stale bot should stay away labels Sep 20, 2022
@cho-m cho-m added CI-long-timeout [DEPRECATED] Use longer GitHub Actions CI timeout. long build Set a long timeout for formula testing labels Sep 20, 2022
@cho-m cho-m added CI-no-fail-fast Continue CI tests despite failing GitHub Actions matrix builds. and removed CI-long-timeout [DEPRECATED] Use longer GitHub Actions CI timeout. labels Sep 20, 2022
@cho-m cho-m force-pushed the sdl12 branch 2 times, most recently from 1a021e9 to b54194c Compare September 20, 2022 03:44
@cho-m cho-m added the CI-long-timeout [DEPRECATED] Use longer GitHub Actions CI timeout. label Sep 20, 2022
@cho-m
Copy link
Member Author

cho-m commented Sep 20, 2022

May want to ignore sdl_sound flat namespace audit failure and fix separately if only issue.

Will note that none of patches look like they will apply and autoreconf will fail without creating AUTHORS/etc files.

@carlocab
Copy link
Member

carlocab commented Sep 20, 2022

May want to ignore sdl_sound flat namespace audit failure and fix separately if only issue.

Will note that none of patches look like they will apply and autoreconf will fail without creating AUTHORS/etc files.

I'm ok to add this to the allowlist if needed.

@carlocab carlocab removed the CI-long-timeout [DEPRECATED] Use longer GitHub Actions CI timeout. label Sep 20, 2022
@cho-m
Copy link
Member Author

cho-m commented Sep 20, 2022

Linux failures. Haven't checked yet if any are from changes here vs GCC/etc migrations

Error: 8 failed steps!
brew install --verbose --build-bottle nuvie
brew install --verbose --build-bottle pce
brew install --verbose --build-bottle pmdmini
brew install --verbose --build-bottle qdae
brew install --verbose --build-bottle supermodel
brew install --verbose --build-bottle sz81
brew install --verbose --build-bottle vecx
brew install --verbose --build-bottle xrick
Error: Process completed with exit code 1.

@carlocab
Copy link
Member

Linux failures. Haven't checked yet if any are from changes here vs GCC/etc migrations

Let's find out: https://github.com/carlocab/workflow-test/actions/runs/3089013194

@cho-m
Copy link
Member Author

cho-m commented Sep 20, 2022

Let's find out: https://github.com/carlocab/workflow-test/actions/runs/3089013194

They all failed, so probably GCC-11 and other changes rather than sdl.

@cho-m
Copy link
Member Author

cho-m commented Sep 21, 2022

Are we okay with merging with current failures and fixing those afterward?

macOS:

  • sdl_sound

Linux:

@cho-m cho-m added the maintainer feedback Additional maintainers' opinions may be needed label Sep 21, 2022
carlocab
carlocab previously approved these changes Sep 22, 2022
Copy link
Member

@carlocab carlocab left a comment

Choose a reason for hiding this comment

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

Ok by me to fix forward.

@BrewTestBot
Copy link
Member

:shipit: @SMillerDev has triggered a merge.

@BrewTestBot
Copy link
Member

⚠️ @SMillerDev bottle publish failed.

@BrewTestBot BrewTestBot dismissed carlocab’s stale review September 22, 2022 08:39

bottle publish failed

Copy link
Member

@carlocab carlocab left a comment

Choose a reason for hiding this comment

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

Needs --no-autosquash

@BrewTestBot
Copy link
Member

:shipit: @SMillerDev has triggered a merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI-no-fail-fast Continue CI tests despite failing GitHub Actions matrix builds. long build Set a long timeout for formula testing maintainer feedback Additional maintainers' opinions may be needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants