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

jj (Jujutsu) 0.4.0 (new formula) #105986

Closed

Conversation

ClashTheBunny
Copy link
Contributor

This adds the latest release as well as the ability to install HEAD. I
used the same structure as Starship, so it should work in all the same
places. I've tested it on Monterey and Debian.

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

@BrewTestBot BrewTestBot added new formula PR adds a new formula to Homebrew/homebrew-core rust Rust use is a significant feature of the PR or issue labels Jul 16, 2022
ClashTheBunny added a commit to ClashTheBunny/jj that referenced this pull request Jul 17, 2022
This adds directions that will work when
Homebrew/homebrew-core#105986 is merged.  If that is
not merged, we can create a tap and adjust these directions accordingly.
ClashTheBunny added a commit to ClashTheBunny/jj that referenced this pull request Jul 17, 2022
This adds directions that will work when
Homebrew/homebrew-core#105986 is merged.  If that is
not merged, we can create a tap and adjust these directions accordingly.
Formula/jj.rb Outdated
Comment on lines 10 to 17
depends_on "openssl@1.1"

on_linux do
depends_on "pkg-config" => :build
end
def install
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
depends_on "openssl@1.1"
on_linux do
depends_on "pkg-config" => :build
end
def install
depends_on "openssl@1.1"
uses_from_macos "zlib"
on_linux do
depends_on "pkg-config" => :build
end
def install

Copy link
Member

Choose a reason for hiding this comment

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

Also, is the OpenSSL dependency needed on macOS? It seems like it might be Linux-only.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the directions for compiling from source, it does suggest installing openssl and adding it to the PKG_CONFIG_PATH. https://github.com/martinvonz/jj#mac

I assume the zlib just uses_from_macos is just being explicit? Just curious why it worked for me without it.

Copy link
Member

Choose a reason for hiding this comment

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

There's currently no linkage with OpenSSL, though, given the brew linkage output:

==>brew linkage --cached jj
System libraries:
  /usr/lib/libSystem.B.dylib
  /usr/lib/libiconv.2.dylib
  /usr/lib/libresolv.9.dylib
  /usr/lib/libz.1.dylib

Are you sure cargo makes use of OpenSSL?

Copy link
Member

Choose a reason for hiding this comment

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

I assume the zlib just uses_from_macos is just being explicit? Just curious why it worked for me without it.

uses_from_macos is for Linux; it does nothing on 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.

@martinvonz, do you remember when openssl is required for macOS? I assume it could be an older version of macOS or something?

Choose a reason for hiding this comment

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

I don't remember if or when it's required for macOS. I added the vendoring of OpenSSL in cac93e27939362fc4a834f5e3654a5bf2eaaf618. That was for musl support on Linux. You can disable the vendored-openssl feature if you want to link against the installed OpenSSL instead on macOS.

Regarding the installation instructions, I think I just forgot to remove the suggestion to install openssl when I made vendored-openssl the default.

(Sorry about the slow response; I'm on vacation and will be for another two weeks.)

Copy link
Member

Choose a reason for hiding this comment

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

We can probably keep OpenSSL as Linux-only, then. We should also disable the vendored-openssl feature, as we want to be able to ship security updates.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, so disabling vendored-openssl results in it linking with the brew openssl (uses_from_macos openssl):

randall@randalls-mbp /opt/homebrew/Library/Taps/homebrew/homebrew-core (jujutsu_new_formula)$ brew linkage --cached jj
System libraries:
  /System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation
  /System/Library/Frameworks/Security.framework/Versions/A/Security
  /System/Library/Frameworks/SystemConfiguration.framework/Versions/A/SystemConfiguration
  /usr/lib/libSystem.B.dylib
  /usr/lib/libiconv.2.dylib
  /usr/lib/libresolv.9.dylib
  /usr/lib/libz.1.dylib
Homebrew libraries:
  /opt/homebrew/opt/openssl@1.1/lib/libcrypto.1.1.dylib (openssl@1.1)
  /opt/homebrew/opt/openssl@1.1/lib/libssl.1.1.dylib (openssl@1.1)
Undeclared dependencies with linkage:
  openssl@1.1

So I've kept it as depends_on openssl with the vendored-openssl removed:

randall@randalls-mbp /opt/homebrew/Library/Taps/homebrew/homebrew-core (jujutsu_new_formula●)$ brew linkage jj 
System libraries:
  /System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation
  /System/Library/Frameworks/Security.framework/Versions/A/Security
  /System/Library/Frameworks/SystemConfiguration.framework/Versions/A/SystemConfiguration
  /usr/lib/libSystem.B.dylib
  /usr/lib/libiconv.2.dylib
  /usr/lib/libresolv.9.dylib
  /usr/lib/libz.1.dylib
Homebrew libraries:
  /opt/homebrew/opt/openssl@1.1/lib/libcrypto.1.1.dylib (openssl@1.1)
  /opt/homebrew/opt/openssl@1.1/lib/libssl.1.1.dylib (openssl@1.1)

The linux linkage reports a lint with the gcc linkage, so I added another depends_on and we're good on both platforms.

❯ brew linkage jj 
System libraries:
  /lib/x86_64-linux-gnu/libc.so.6
  /lib/x86_64-linux-gnu/libdl.so.2
  /lib/x86_64-linux-gnu/libm.so.6
  /lib/x86_64-linux-gnu/libpthread.so.0
  /lib64/ld-linux-x86-64.so.2
Homebrew libraries:
  /home/randall/.linuxbrew/lib/gcc/11/libgcc_s.so.1 (gcc)
  /home/randall/.linuxbrew/opt/openssl@1.1/lib/libcrypto.so.1.1 (openssl@1.1)
  /home/randall/.linuxbrew/opt/openssl@1.1/lib/libssl.so.1.1 (openssl@1.1)
  /home/randall/.linuxbrew/opt/zlib/lib/libz.so.1 (zlib)

Formula/jj.rb Outdated Show resolved Hide resolved
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.

While we're waiting to hear about the OpenSSL dependency, let's clean up one small nit (unless you feel strongly otherwise).

Formula/jj.rb Outdated Show resolved Hide resolved
@carlocab carlocab added the needs response Needs a response from the issue/PR author label Jul 20, 2022
@github-actions
Copy link
Contributor

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 Aug 15, 2022
@martinvonz
Copy link

@ClashTheBunny, let me know if there's anything I can do to help

@github-actions github-actions bot removed the stale No recent activity label Aug 15, 2022
ClashTheBunny and others added 4 commits August 22, 2022 16:07
This adds the latest release as well as the ability to install HEAD.  I
used the same structure as Starship, so it should work in all the same
places. I've tested it on Monterey and Debian.
Co-authored-by: Carlo Cabrera <30379873+carlocab@users.noreply.github.com>
Co-authored-by: Carlo Cabrera <30379873+carlocab@users.noreply.github.com>
@ClashTheBunny
Copy link
Contributor Author

I did want to make sure that there's no policy that the jj --version command outputs a sha or something other than the last release for the --HEAD build.

Also, should I squash away all the little commits, or is Github trustworthy with squashing? Since it's a brand new file, I assume it will do fine...

@github-actions
Copy link
Contributor

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 Sep 13, 2022
@ClashTheBunny
Copy link
Contributor Author

@carlocab, is there anything else that needs to happen here? I think it's ready to go.

@github-actions github-actions bot removed the stale No recent activity label Sep 13, 2022
@BrewTestBot
Copy link
Member

:shipit: @jonchang has triggered a merge.

ClashTheBunny added a commit to ClashTheBunny/jj that referenced this pull request Oct 16, 2022
This adds directions that will work when
Homebrew/homebrew-core#105986 is merged.  If that is
not merged, we can create a tap and adjust these directions accordingly.
ClashTheBunny added a commit to martinvonz/jj that referenced this pull request Oct 18, 2022
This adds directions that will work when
Homebrew/homebrew-core#105986 is merged.  If that is
not merged, we can create a tap and adjust these directions accordingly.
@ClashTheBunny ClashTheBunny deleted the jujutsu_new_formula branch October 18, 2022 01:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs response Needs a response from the issue/PR author new formula PR adds a new formula to Homebrew/homebrew-core rust Rust use is a significant feature of the PR or issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants