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

cleaner: remove *.tbd #16355

Merged
merged 1 commit into from Dec 19, 2023
Merged

cleaner: remove *.tbd #16355

merged 1 commit into from Dec 19, 2023

Conversation

AkihiroSuda
Copy link
Contributor

@AkihiroSuda AkihiroSuda commented Dec 18, 2023

For #16355 (comment)

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew typecheck with your changes locally?
  • Have you successfully run brew tests with your changes locally?

Needed by:

@MikeMcQuaid
Copy link
Member

@AkihiroSuda Do you have any examples beyond Homebrew/homebrew-core#157617 for things that might need this? Thanks!

@AkihiroSuda
Copy link
Contributor Author

@AkihiroSuda Do you have any examples beyond Homebrew/homebrew-core#157617 for things that might need this? Thanks!

I don't know.

@Bo98
Copy link
Member

Bo98 commented Dec 18, 2023

Bit of a weird one given shipping both a .tbd and a .dylib seems pointless - Apple ship .tbd to avoid shipping .dylib in their SDKs.

It's not wrong per se but instead rather meaningless.

It seems to be a new CMake feature. We could clean them like we do for .la files:

# * removes `.la` files

if path.extname == ".la" || PERL_BASENAMES.include?(path.basename.to_s)

@MikeMcQuaid
Copy link
Member

It seems to be a new CMake feature. We could clean them like we do for .la files:

This seems like a good idea until we have a use for them 👍🏻

@AkihiroSuda
Copy link
Contributor Author

I understand that *.tbd files are not expected to co-exist with *.dylib, but I fear that removing them may potentially break something that does not follow the standard

@carlocab
Copy link
Member

carlocab commented Dec 19, 2023

I fear that removing them may potentially break something that does not follow the standard

I'd rather report those to the relevant upstream project as a bug instead of preemptively working around them by always keeping the *.tbd files.

There's another formula where we go out of our way to remove them because they break the bottle.

https://github.com/Homebrew/homebrew-core/blob/3d8c3e1ab8652dd836f7a7b422e38578393cbe60/Formula/d/duck.rb#L157

@Bo98
Copy link
Member

Bo98 commented Dec 19, 2023

Yeah it should be safe to treat them exactly as we do .la files.

For CMake, it happens if ENABLE_EXPORTS ON is set, but that also does other things like linkable executables so it's not quite as simple as turning it off (not that we can anyway without patching). wasmedge is not intentionally adding .tbd files as they did not exist prior to CMake 3.27 just a few months ago.

For Homebrew#16355 (comment)

Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>
@AkihiroSuda AkihiroSuda changed the title VALID_LIBRARY_EXTENSIONS: allow *.tbd cleaner: remove *.tbd Dec 19, 2023
@AkihiroSuda
Copy link
Contributor Author

Updated PR to remove *.tbd

Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Thanks so much for your first contribution! Without people like you submitting PRs we couldn't run this project. You rock, @AkihiroSuda!

@MikeMcQuaid MikeMcQuaid merged commit db01274 into Homebrew:master Dec 19, 2023
24 checks passed
@github-actions github-actions bot added the outdated PR was locked due to age label Jan 19, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants