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

sleef: Also build inline headers #88923

Closed
wants to merge 1 commit into from

Conversation

armanbilge
Copy link
Contributor

As described in https://sleef.org/compile.xhtml#inline.

This generates additional header files (rather than changing the standard ones). So, IMHO I see no reason not to enable it, since they can simply be ignored by users who don't need them. I assume this option disabled by default because it increases the build time.

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

As described in https://sleef.org/compile.xhtml#inline.

This generates _additional_ header files (rather than modifying the standard ones). So, IMHO I see no reason not to enable it by default, since they can simply be ignored by users who don't need them.

I assume this option disabled by default because it increases the build time.
@BrewTestBot BrewTestBot added the no ARM bottle Formula has no ARM bottle label Nov 7, 2021
@carlocab carlocab added the CI-no-fail-fast Continue CI tests despite failing GitHub Actions matrix builds. label Nov 7, 2021
@armanbilge
Copy link
Contributor Author

I don't think the CI failures are related to my change, but I could be mistaken. I'm not sure if this formula supports ARM?

The last PR for sleef appeared to pass CI, but it didn't run any tests on ARM:

@carlocab
Copy link
Member

carlocab commented Nov 7, 2021

Why aren't the inline headers built by default?

The last PR for sleef appeared to pass CI, but it didn't run any tests on ARM:

That's true, but that PR was made in September last year. ARM macOS was released in November.

@armanbilge
Copy link
Contributor Author

Why aren't the inline headers built by default?

I don't really have a good answer for this. This is the documentation I can find regarding this feature:
https://sleef.org/compile.xhtml#inline
https://sleef.org/additional.xhtml#inline
My best guess is that it makes the build take longer, so it is disabled by default. These additional header files would not interfere at all with ordinary use of the library.

That's true, but that PR was made in September last year. ARM macOS was released in November.

Gotcha. So, what happens if this library doesn't support ARM?

@carlocab
Copy link
Member

carlocab commented Nov 7, 2021

We should find out why the developers don't enable this by default -- they probably have a good reason for it. It doesn't seem like this should add too much extra compilation time.

As for ARM, we can ignore the build failure for now, but it would be good to report this upstream so that they're aware of the incompatibility (if they're not already).

@armanbilge
Copy link
Contributor Author

armanbilge commented Nov 7, 2021

We should find out why the developers don't enable this by default -- they probably have a good reason for it. It doesn't seem like this should add too much extra compilation time.

Actually, I think that is exactly the reason. See: shibatch/sleef#283 (comment)

Yes, running time in CI is increased.
If that is a problem, we can just turn off generation.


As for ARM, we can ignore the build failure for now, but it would be good to report this upstream so that they're aware of the incompatibility (if they're not already).

There is this open issue:

Does this address your concerns?

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.

Sounds good to me. Thanks for your contribution, @armanbilge!

@carlocab
Copy link
Member

carlocab commented Nov 8, 2021

Incidentally, the error reported at shibatch/sleef#425 is slightly different, but I've posted there with the errors we encountered.

@BrewTestBot
Copy link
Member

:shipit: @carlocab has triggered a merge.

@github-actions github-actions bot added the outdated PR was locked due to age label Dec 9, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CI-no-fail-fast Continue CI tests despite failing GitHub Actions matrix builds. no ARM bottle Formula has no ARM bottle outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants