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

linkage_checker: deprecate linkage to libcrypt.so.1 #13151

Merged
merged 2 commits into from Apr 27, 2022

Conversation

Bo98
Copy link
Member

@Bo98 Bo98 commented Apr 15, 2022

This isn't something to be merged yet (there's not even a libxcrypt in homebrew-core yet), but this more formally proposes what I discussed in Slack which is that we should follow what other Linux distributions have done and stop depending on Glibc's libcrypt and replace with libxcrypt (which we'll provide as libcrypt.so.2).

@BrewTestBot
Copy link
Member

Review period will end on 2022-04-18 at 19:45:48 UTC.

@BrewTestBot BrewTestBot added waiting for feedback Merging is blocked until sufficient time has passed for review and removed waiting for feedback Merging is blocked until sufficient time has passed for review labels Apr 15, 2022
@BrewTestBot BrewTestBot added waiting for feedback Merging is blocked until sufficient time has passed for review and removed waiting for feedback Merging is blocked until sufficient time has passed for review labels Apr 16, 2022
@BrewTestBot BrewTestBot added waiting for feedback Merging is blocked until sufficient time has passed for review and removed waiting for feedback Merging is blocked until sufficient time has passed for review labels Apr 17, 2022
Comment on lines 32 to 33
opoo "Linkage to libcrypt.so.1 is deprecated and will fail linkage checks in Homebrew 3.??.0.\n" \
"Migrate to libcrypt.so.2 in the libxcrypt formula instead."
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
opoo "Linkage to libcrypt.so.1 is deprecated and will fail linkage checks in Homebrew 3.??.0.\n" \
"Migrate to libcrypt.so.2 in the libxcrypt formula instead."
opoo "Linkage to libcrypt.so.1 is deprecated! \n" \
"Migrate to libcrypt.so.2 in the libxcrypt formula instead."

or ideally just:

Suggested change
opoo "Linkage to libcrypt.so.1 is deprecated and will fail linkage checks in Homebrew 3.??.0.\n" \
"Migrate to libcrypt.so.2 in the libxcrypt formula instead."
odeprecated "linkage to libcrypt.so.1", "link to libcrypt.so.2 from the libxcrypt formula"

Copy link
Member Author

@Bo98 Bo98 Apr 18, 2022

Choose a reason for hiding this comment

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

I intentionally do not want odeprecated's raise-on-HOMEBREW_DEVELOPER behaviour. I'd like the warning to appear in our CI but not actually fail linkage yet. The opt-in raise-to-error behaviour in this case is handled by --strict (see broken_library_linkage?).

I can see us going straight from this to removing libcrypt.so.1 from the allowed library whitelist. Or maybe actually have a stepping stone of some opt-in raise-to-error env once we migrate homebrew-core but haven't removed it here yet (but not as global as HOMEBREW_DEVELOPER which affects everyone using test bot which defeats the point of the warning grace period really).

Copy link
Member Author

Choose a reason for hiding this comment

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

Or maybe actually have a stepping stone of some opt-in raise-to-error

Modified it now with that.

Copy link
Member

Choose a reason for hiding this comment

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

Could this use odeprecated when --strict is passed? Feels a bit weird to have a deprecation that's not using it. Alternatively, a flag for odeprecated to make it require more than HOMEBREW_DEVELOPER in this case?

Copy link
Member Author

Choose a reason for hiding this comment

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

Adjusted this now. The English is a bit weird since it says "Calling linkage", but other than that it works.

@BrewTestBot BrewTestBot added waiting for feedback Merging is blocked until sufficient time has passed for review and removed waiting for feedback Merging is blocked until sufficient time has passed for review labels Apr 18, 2022
@BrewTestBot BrewTestBot added waiting for feedback Merging is blocked until sufficient time has passed for review and removed waiting for feedback Merging is blocked until sufficient time has passed for review labels Apr 18, 2022
@BrewTestBot BrewTestBot added waiting for feedback Merging is blocked until sufficient time has passed for review and removed waiting for feedback Merging is blocked until sufficient time has passed for review labels Apr 18, 2022
@BrewTestBot
Copy link
Member

Review period ended.

@Bo98 Bo98 removed the do not merge label Apr 27, 2022
@Bo98 Bo98 marked this pull request as ready for review April 27, 2022 14:42
@Bo98 Bo98 merged commit e6cb1f1 into Homebrew:master Apr 27, 2022
@Bo98 Bo98 deleted the libcrypt-1-deprecation branch April 27, 2022 15:00
@github-actions github-actions bot added the outdated PR was locked due to age label May 28, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 28, 2022
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

3 participants