Skip to content

metanorma 1.12.8 (new formula) #225342

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

metanorma 1.12.8 (new formula) #225342

wants to merge 1 commit into from

Conversation

alex-sc
Copy link
Contributor

@alex-sc alex-sc commented May 31, 2025

This PR add a bottle for metanorma, specifically the metanorma-cli gem. More context here

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

@github-actions github-actions bot added new formula PR adds a new formula to Homebrew/homebrew-core java Java use is a significant feature of the PR or issue ruby Ruby use is a significant feature of the PR or issue labels May 31, 2025
@alex-sc alex-sc force-pushed the master branch 11 times, most recently from 5069a1a to 01960d6 Compare June 1, 2025 09:33
@alex-sc
Copy link
Contributor Author

alex-sc commented Jun 1, 2025

I'll try to fix the Linux issue tomorrow

@alex-sc alex-sc force-pushed the master branch 2 times, most recently from 5c8dfc3 to bb80ad9 Compare June 3, 2025 04:48
@alex-sc
Copy link
Contributor Author

alex-sc commented Jun 3, 2025

Seems to be fixed

Comment on lines 32 to 43
# Install sqlite3 with brew's libsqlite3
system "gem", "install", "sqlite3", "-v", "1.7.3", "--no-document",
"--platform=ruby",
"--",
"--enable-system-libraries",
"--with-sqlite3-include=#{Formula["sqlite"].opt_include}",
"--with-sqlite3-lib=#{Formula["sqlite"].opt_lib}"

# Install pngcheck with brew's zlib (libz.so.1)
ENV.append "CFLAGS", "-I$(brew --prefix zlib)/include"
ENV.append "LDFLAGS", "-L$(brew --prefix zlib)/lib -Wl,-rpath,$(brew --prefix zlib)/lib"
ENV.append "PKG_CONFIG_PATH", "$(brew --prefix zlib)/lib/pkgconfig"
Copy link
Member

Choose a reason for hiding this comment

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

This would require a real zlib dependency since there is currently no dependency on brewed zlib for macos

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@SMillerDev

Thanks! Before these changes the Linux build was failing with the below errors, so I've switched to the brew libsqlite3 and (as I thought) brew libz. What would you suggest on libz?

linkage --test metanorma

  Unwanted system libraries:
    /lib/x86_64-linux-gnu/libz.so.1

brew linkage --reverse metanorma

/lib/x86_64-linux-gnu/libz.so.1
  libexec/gems/pngcheck-0.3.1-x86_64-linux/lib/pngcheck/pngcheck.so
  libexec/gems/sqlite3-1.7.3-x86_64-linux/lib/sqlite3/3.0/sqlite3_native.so
  libexec/gems/sqlite3-1.7.3-x86_64-linux/lib/sqlite3/3.1/sqlite3_native.so
  libexec/gems/sqlite3-1.7.3-x86_64-linux/lib/sqlite3/3.2/sqlite3_native.so
  libexec/gems/sqlite3-1.7.3-x86_64-linux/lib/sqlite3/3.3/sqlite3_native.so

Copy link
Member

Choose a reason for hiding this comment

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

The fix seems correct for linux, but only for linux.

libexec/"bin",
PATH: [libexec/"bin", "$PATH"].join(":"),
GEM_HOME: ENV["GEM_HOME"],
JAVA_HOME: Language::Java.java_home("1.8+"),
Copy link
Member

Choose a reason for hiding this comment

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

I think overridable java_home is a better choice here

@github-actions github-actions bot added the autosquash Automatically squash pull request commits according to Homebrew style. label Jun 4, 2025
@github-actions github-actions bot removed the autosquash Automatically squash pull request commits according to Homebrew style. label Jun 4, 2025
@alex-sc
Copy link
Contributor Author

alex-sc commented Jun 4, 2025

@SMillerDev I think I've address the issues. Please review again when you have a chance. Thanks!

Comment on lines 28 to 40
# Install sqlite3 with brew's libsqlite3
system "gem", "install", "sqlite3", "-v", "1.7.3", "--no-document",
"--platform=ruby",
"--",
"--enable-system-libraries",
"--with-sqlite3-include=#{Formula["sqlite"].opt_include}",
"--with-sqlite3-lib=#{Formula["sqlite"].opt_lib}"

# Install pngcheck with brew's zlib (libz.so.1)
ENV.append "CFLAGS", "-I$(brew --prefix zlib)/include"
ENV.append "LDFLAGS", "-L$(brew --prefix zlib)/lib -Wl,-rpath,$(brew --prefix zlib)/lib"
ENV.append "PKG_CONFIG_PATH", "$(brew --prefix zlib)/lib/pkgconfig"
system "gem", "install", "pngcheck", "--no-document", "--platform=ruby"
Copy link
Member

Choose a reason for hiding this comment

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

Are these not needed on macOS? And does this mean they are downloaded on the fly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sqlite3 and pngcheck are installed by the main gem by default. These are overrides to deal with the Linux libraries

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 does this mean they are downloaded on the fly?

Yes. I guess that's an issue?

Should we list all the 200+ gems explicitly instead? I'm not sure if that's feasible/doable

We do have a binary distributions, but that is not going to work in homebrew-core (https://github.com/metanorma/homebrew-metanorma/blob/main/Formula/metanorma.rb)

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, either it should use resources for the gems, or use a lock file to ensure that it only downloads gems we know of.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@SMillerDev Thanks. I look into it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MikeMcQuaid The bundle install approach didn't work for some reason, so I came up with a different solution using gem install

I agree bundle install is better and clearer. Let me revisit...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MikeMcQuaid

I've reworked the formula to use Gemlock. Since the Gemlock is not part of the tarball, I've added it to the releases as a separate artifact for now. I'll talk to the maintainers if it's possible to adjust the tarball in future releases

Now it's failing with GitHub repository not notable enough (<30 forks, <30 watchers and <75 stars) which is undestandable since the metanorma-cli in not the main repo. https://github.com/metanorma/metanorma is

Copy link
Member

Choose a reason for hiding this comment

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

@alex-sc We'd need to use the upstream Gemfile and have upstream accept a PR to add a Gemfile.lock in there before we could accept this, sorry. We don't need it in a stable release as can be applied as a patch using an upstream merged commit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MikeMcQuaid Sure, I understand. I suppose it makes sense to close this PR for now

Copy link
Member

Choose a reason for hiding this comment

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

@alex-sc I'm happy waiting a bit if you are! Making it draft for now instead.

libexec/"bin",
PATH: [libexec/"bin", "$PATH"].join(":"),
GEM_HOME: ENV["GEM_HOME"],
JAVA_HOME: ENV["JAVA_HOME"] || Language::Java.java_home("1.8+"),
Copy link
Member

Choose a reason for hiding this comment

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

You should use the overridable Java home method for this because the current way won't work with CI

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@SMillerDev You mean this, right?

JAVA_HOME: Language::Java.overridable_java_home_env,

Copy link
Member

Choose a reason for hiding this comment

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

Yes

@alex-sc
Copy link
Contributor Author

alex-sc commented Jun 9, 2025

Yeah, either it should use resources for the gems, or use a lock file to ensure that it only downloads gems we know of.

@SMillerDev

I've refactored the code to use vendored-gems.tar.gz and install all the required gems 'offline'. Please have a look

@alex-sc alex-sc force-pushed the master branch 7 times, most recently from 1cc20f9 to dae7e40 Compare June 11, 2025 06:17
Co-Authored-By: Sean Molenaar <[email protected]>
@MikeMcQuaid MikeMcQuaid marked this pull request as draft June 12, 2025 12:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
java Java use is a significant feature of the PR or issue new formula PR adds a new formula to Homebrew/homebrew-core ruby Ruby use is a significant feature of the PR or issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants