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

flint 3.1.3 #166490

Merged
merged 2 commits into from
May 18, 2024
Merged

flint 3.1.3 #166490

merged 2 commits into from
May 18, 2024

Conversation

edgarcosta
Copy link
Contributor

@edgarcosta edgarcosta commented Mar 18, 2024

  • 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 HOMEBREW_NO_INSTALL_FROM_API=1 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 HOMEBREW_NO_INSTALL_FROM_API=1 brew install --build-from-source <formula>)? If this is a new formula, does it pass brew audit --new <formula>?

I'm not sure what you would like me to do about:

flint

  • line 39, col 5: Formulae should be verified as having support for runtime hardware detection before using ENV.runtime_cpu_detection.
    Error: 1 problem in 1 formula detected.

depends on #166574

@github-actions github-actions bot added the automerge-skip `brew pr-automerge` will skip this pull request label Mar 18, 2024
@SMillerDev
Copy link
Member

Can you make disabling ARB a different PR? That should solve the issue I believe

@edgarcosta edgarcosta changed the title uptading FLINT (git, download, and bumping version) and disable arb uptading FLINT (git, download, and bumping version) Mar 19, 2024
@github-actions github-actions bot added the autosquash Automatically squash pull request commits according to Homebrew style. label Mar 19, 2024
@edgarcosta edgarcosta force-pushed the flint3 branch 2 times, most recently from 6d00995 to 9fec260 Compare March 19, 2024 10:59
@github-actions github-actions bot removed the autosquash Automatically squash pull request commits according to Homebrew style. label Mar 19, 2024
@edgarcosta edgarcosta changed the title uptading FLINT (git, download, and bumping version) updating FLINT (git, download, and bumping version) Mar 19, 2024
@edgarcosta edgarcosta force-pushed the flint3 branch 2 times, most recently from 89cf498 to 56a2ee6 Compare March 19, 2024 13:53
@github-actions github-actions bot removed the automerge-skip `brew pr-automerge` will skip this pull request label Mar 19, 2024
@edgarcosta
Copy link
Contributor Author

#166574 has been merged, so no longer issues with arb, but I still do not know how to address

Formulae should be verified as having support for runtime hardware detection before using ENV.runtime_cpu_detection.

Formula/f/flint.rb Outdated Show resolved Hide resolved
Formula/f/flint.rb Outdated Show resolved Hide resolved
@SMillerDev SMillerDev changed the title updating FLINT (git, download, and bumping version) flint 3.1.2 Mar 22, 2024
@github-actions github-actions bot added the autosquash Automatically squash pull request commits according to Homebrew style. label Mar 22, 2024
@github-actions github-actions bot removed the autosquash Automatically squash pull request commits according to Homebrew style. label Mar 22, 2024
@edgarcosta
Copy link
Contributor Author

@SMillerDev, thanks for the suggestion, I have also incorporated @albinahlback suggestion, but I still do not know how to address:

Formulae should be verified as having support for runtime hardware detection before using ENV.runtime_cpu_detection.

@chenrui333
Copy link
Member

cc @caarlos0 for thoughts.

@caarlos0
Copy link
Sponsor Contributor

caarlos0 commented Apr 3, 2024

cc @caarlos0 for thoughts.

wrong cc maybe? I know nothing about flint or the underlying issue

@edgarcosta
Copy link
Contributor Author

The underlying issue is the warning:

Formulae should be verified as having support for runtime hardware detection before using ENV.runtime_cpu_detection

@github-actions github-actions bot added the automerge-skip `brew pr-automerge` will skip this pull request label Apr 4, 2024
@edgarcosta
Copy link
Contributor Author

I figured it out; one needs to add flint to style_exceptions/runtime_cpu_detection_allowlist.json

@edgarcosta edgarcosta force-pushed the flint3 branch 2 times, most recently from ae8d289 to fff22f6 Compare April 4, 2024 22:05
@edgarcosta
Copy link
Contributor Author

I am honestly confused about the error message,

No `cpuid` instruction detected. flint should not use `ENV.runtime_cpu_detection`.

as FLINT uses cpuid, see: https://github.com/flintlib/flint/blob/main/config/config.guess#L797

@github-actions github-actions bot added the autosquash Automatically squash pull request commits according to Homebrew style. label May 10, 2024
@edgarcosta edgarcosta force-pushed the flint3 branch 2 times, most recently from 034e728 to e699748 Compare May 10, 2024 18:43
@edgarcosta edgarcosta changed the title flint 3.1.2 flint 3.1.3 May 13, 2024
FLINT has changed repo to https://github.com/flintlib/flint
This also now has releases, and the current version is 3.1.3
Setting livecheck to use github_latest
Co-authored-by: Sean Molenaar <SMillerDev@users.noreply.github.com>
@github-actions github-actions bot removed the autosquash Automatically squash pull request commits according to Homebrew style. label May 13, 2024
@edgarcosta
Copy link
Contributor Author

I think I fixed everything now, and this is ready to review

uses_from_macos "m4" => :build

def install
# to build against NTL
Copy link
Contributor

Choose a reason for hiding this comment

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

NTL doesn't appear anywhere else in the formula now, is this correct?

Choose a reason for hiding this comment

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

Yes. We have a header-only interface.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

and the NTL interface header is a C++ header

args << "--enable-arch=#{Hardware.oldest_cpu}"
elsif Hardware::CPU.avx2?
# TODO: enable avx512 support
args << "--enable-avx2"
Copy link
Contributor

Choose a reason for hiding this comment

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

Flint doesn't use BLAS yet, right? If it did, Apple's Accelerate framework has SIMD vector operations that replace avx: https://developer.apple.com/documentation/accelerate/simd

Copy link

@albinahlback albinahlback May 14, 2024

Choose a reason for hiding this comment

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

We do use it in multiplication for the nmod_mat and fmpz_mat modules.

Edit: BLAS, that is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

BLAS is optional; we could compile against it and add it as a dependency.
@albinahlback and @fredrik-johansson, is this something that we want?

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.

Thanks; great work!

Copy link
Contributor

🤖 An automated task has requested bottles to be published to this PR.

@github-actions github-actions bot added the CI-published-bottle-commits The commits for the built bottles have been pushed to the PR branch. label May 18, 2024
@carlocab carlocab removed the stale No recent activity label May 18, 2024
@BrewTestBot BrewTestBot added this pull request to the merge queue May 18, 2024
Merged via the queue into Homebrew:master with commit bc17c84 May 18, 2024
16 checks passed
@dimpase
Copy link
Contributor

dimpase commented May 21, 2024

pkg-config --modversion flint returns 3.1.0 instead of 3.1.3.
For some reason the version is not bumped up, not sure it's here or upstream.

@cho-m
Copy link
Member

cho-m commented May 21, 2024

pkg-config --modversion flint returns 3.1.0 instead of 3.1.3. For some reason the version is not bumped up, not sure it's here or upstream.

Probably should open issue upstream for CI workflow relating to release tarballs to make sure VERSION file is modified before bootstrap.sh is run.

The problem looks like reverse is happening so configure contains:

PACKAGE_VERSION='3.1.0'
PACKAGE_STRING='FLINT 3.1.0'
...
FLINT_VERSION_FULL=3.1.0

https://github.com/flintlib/flint/blob/main/.github/workflows/release.yml#L88-L93

      - name: "Bootstrap"
        run: |
          ./bootstrap.sh

      - name: "Create source archive"
        run: dev/make_dist.sh ${FLINT_VERSION}

https://github.com/flintlib/flint/blob/main/dev/make_dist.sh#L39-L40

# update VERSION file
printf $flint_version > VERSION

@edgarcosta
Copy link
Contributor Author

I have submitted two PRs to fix the problem upstream: flintlib/flint#1993 and flintlib/flint#1994 (which FLINT releases if 3.1.4 will exist)

@carlocab
Copy link
Member

Thanks! Feel free to open a PR that backports those patches to the current version of the formula.

@github-actions github-actions bot added the outdated PR was locked due to age label Jun 24, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CI-published-bottle-commits The commits for the built bottles have been pushed to the PR branch. outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants