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

Allow formulae to prevent optimization level stripping #11201

Closed
wants to merge 1 commit into from

Conversation

carlocab
Copy link
Member

@carlocab carlocab commented Apr 20, 2021

  • 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?

Some upstream build systems require finer control over the optimisation
level. For example, libgcrypt sets -O0 when building its
jitter-based entropy collector (jent), but -O2 otherwise.

This allows formulae to set ENV.permit_optimization_level in the
install method to prevent the stripping of -O[0-9zs] flags. In particular,
it will allow us to build libgcrypt with the jitter-based entropy collector
which, as far as I can tell, is a valuable additional source of entropy. (Not
an expert though. But I assume there's a good reason upstream build this
unless you explicitly disable it.)

While we're here, clean up an old comment referencing ENV.libstdcxx.

Some upstream build systems require finer control over the optimization
level. For example, `libgcrypt` sets `-O0` when building its
jitter-based entropy collector, but `-O2` otherwise.

This allows formulae to set `ENV.permit_optimization_level` in the
install method to prevent the stripping of `-O[0-9zs]` flags.

While we're here, clean up an old comment referencing `ENV.libstdcxx`.
@BrewTestBot
Copy link
Member

Review period will end on 2021-04-21 at 22:33:15 UTC.

@BrewTestBot BrewTestBot added the waiting for feedback Merging is blocked until sufficient time has passed for review label Apr 20, 2021
@carlocab
Copy link
Member Author

I'll admit that I'm not sure how I feel about making these changes to superenv for one formula. On the other hand, it's an important formula, and has security-relevant consequences.

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.

I'll admit that I'm not sure how I feel about making these changes to superenv for one formula

I'm definitely opposed, I'm afraid.

What happens with the status quo? Does it fail to build? Does it fail to build the relevant component? Can we make use of the existing ENV.Ox methods to do any of this?

@carlocab
Copy link
Member Author

What happens with the status quo? Does it fail to build? Does it fail to build the relevant component? Can we make use of the existing ENV.Ox methods to do any of this?

With the status quo, we need to disable the jent module. The formula does this by passing --disable-jent-support to configure. Without this flag, libgcrypt will fail to build with the error message

./jitterentropy-base.c:60:4: error: "The CPU Jitter random number generator must not be compiled with optimizations. Use the compiler switch -O0"

We can build the jent module without making any changes to superenv by using ENV.O0 in the install method. This was previously entertained and rejected. See Homebrew/homebrew-core#15750.

So our alternatives to not making this change are:

  1. Not building the jent module, which leaves libgcrypt without an entropy source that the libgcrypt developers find useful enough to add by default;
  2. Build all of libgcrypt with ENV.O0; or,
  3. Find a way to build the jent module separately from the rest of libgcrypt so that ENV.O0 can be used.

I don't think either of 1 or 2 are good options. I've looked into the third, and I'm not sure it's possible. It likely involves patching to the build system that I am sceptical we would be able to upstream (which would also make it a not good option).

Making changes to superenv seems to me like it would be the least bad option here.

However, I will keep looking into option 3. It might be better than my initial impression suggests it is.

@MikeMcQuaid
Copy link
Member

  • Not building the jent module, which leaves libgcrypt without an entropy source that the libgcrypt developers find useful enough to add by default;

This is the current default/status quo, right?

3. Find a way to build the jent module separately from the rest of libgcrypt so that ENV.O0 can be used.

I don't think either of 1 or 2 are good options. I've looked into the third, and I'm not sure it's possible. It likely involves patching to the build system that I am sceptical we would be able to upstream (which would also make it a not good option).

This seems ideal to me if possible.

Making changes to superenv seems to me like it would be the least bad option here.

I'm the opposite. I hate patching software in Homebrew but I think it's preferable rather than introducing a new DSL and the relevant superenv logic for a single formula, particularly considering the current status quo doesn't seem too awful.

@carlocab
Copy link
Member Author

  • Not building the jent module, which leaves libgcrypt without an entropy source that the libgcrypt developers find useful enough to add by default;

This is the current default/status quo, right?

It's what the formula does currently, but it's not the default build configuration. We need to pass --disable-jent-support to disable it.

  1. Find a way to build the jent module separately from the rest of libgcrypt so that ENV.O0 can be used.
    I don't think either of 1 or 2 are good options. I've looked into the third, and I'm not sure it's possible. It likely involves patching to the build system that I am sceptical we would be able to upstream (which would also make it a not good option).

This seems ideal to me if possible.

Ok, will look into this some more then.

@carlocab
Copy link
Member Author

I had forgotten the fourth option, which I did contemplate (but don't remember why I didn't pursue it): make superenv smarter about handling -O flags.

This will avoid the need for:

  • a special DSL for a single formula; and/or,
  • patches to libgcrypt.

I think I can make it a change I'd feel good about. Will work on a PR.

Thanks for the motivation here, Mike.

@MikeMcQuaid
Copy link
Member

I had forgotten the fourth option, which I did contemplate (but don't remember why I didn't pursue it): make superenv smarter about handling -O flags.

@carlocab To save you some work: mind pitching it here and we can discuss it?

@carlocab
Copy link
Member Author

Just to explain my radio silence here: your last comment read a little like (completely warranted) scepticism to me, so I started second-guessing myself. Taking the time to mull over a proper solution here. Will be back with one in the next few days.

@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@github-actions github-actions bot added the stale No recent activity label May 20, 2021
@BrewTestBot BrewTestBot removed the waiting for feedback Merging is blocked until sufficient time has passed for review label May 23, 2021
@BrewTestBot
Copy link
Member

Review period ended.

@github-actions github-actions bot closed this May 31, 2021
@github-actions github-actions bot added the outdated PR was locked due to age label Jul 1, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 1, 2021
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 stale No recent activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants