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

teleport 16.0.1 #175830

Closed
wants to merge 4 commits into from
Closed

teleport 16.0.1 #175830

wants to merge 4 commits into from

Conversation

itsdalmo
Copy link

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

brew audit fails with the following message:

teleport
  * line 32, col 3: Formulae in homebrew/core should use 'depends_on "rust" => build' instead of 'depends_on "rustup-init" => :build'.
  * line 46, col 12: Formula in homebrew/core should not use `rustup-init` at build-time.
Error: 2 problems in 1 formula detected.

But if I swap from rustup-init to rust, the build fails because wasm-pack cannot find the wasm32-unknown-unknown target. As such I stuck with rustup-init and added a comment which explains why (I believe) it is required.

@github-actions github-actions bot added go Go use is a significant feature of the PR or issue nodejs Node or npm use is a significant feature of the PR or issue labels Jun 27, 2024
Copy link
Contributor

Thanks for contributing to Homebrew! 🎉 It looks like you're having trouble with a CI failure. See our contribution guide for help. You may be most interested in the section on dealing with CI failures. You can find the CI logs in the Checks tab of your pull request.

@SMillerDev
Copy link
Member

  # github.com/gravitational/teleport/tool/teleport
  /opt/homebrew/Cellar/go/1.22.4/libexec/pkg/tool/darwin_arm64/link: running clang failed: exit status 1
  ld: library not found for -ld_classic

@SMillerDev SMillerDev added the CI-no-fail-fast Continue CI tests despite failing GitHub Actions matrix builds. label Jun 28, 2024
@SMillerDev
Copy link
Member

Thanks for the PR, I agree we're stuck with rustup here. I've kicked off another CI run to see if it works on other macOS versions.

@itsdalmo
Copy link
Author

Sorry, I force pushed a change (before I saw that you had commented and kicked off another CI run) in an attempt to fix the -ld_classic error you pasted above. It seems like the flag was added to the teleport builds 2 weeks ago and backported to version 16 (as well as 14 and 15).

Here is the change:

ifeq ("$(OS)-$(ARCH)","darwin-arm64")
# Temporary link flags due to changes in Apple's linker
# https://github.com/golang/go/issues/67854
GO_LDFLAGS += -extldflags=-ld_classic
endif

So I suppose this makes xcode a build dependency on darwin, but only on arm64? Need to figure out how to represent this in the formula though.

Formula/t/teleport.rb Outdated Show resolved Hide resolved
@github-actions github-actions bot added the autosquash Automatically squash pull request commits according to Homebrew style. label Jun 28, 2024
@SMillerDev
Copy link
Member

Linux runs out of disk space and I suspect macOS 12 doesn't have a new enough CLang to recognise that flag.

@itsdalmo
Copy link
Author

itsdalmo commented Jun 28, 2024

Yep, seems like the only solution is to mark macOS 12 as broken and not build it?
(edit: Seems like the flag was introduced in Xcode 15, so I have set that as the minimum version)

@SMillerDev
Copy link
Member

Could you ask upstream if building with Xcode 14 is still supposed to be supported? Because that would clarify if we should restrict it or patch it.

@itsdalmo
Copy link
Author

itsdalmo commented Jul 1, 2024

According to the installation instructions, the minimal version of teleport is v10.15+ (Catalina):
https://goteleport.com/docs/installation

As we've noticed, they use a build flag that requires Xcode 15, which in turn requires macOS 13:
https://developer.apple.com/support/xcode

And as far as I can tell, they only test/build on macos 13:
https://github.com/gravitational/teleport/blob/master/.github/workflows/build-macos.yaml#L22

To me this means that xcode 15 is required to build the binaries, but the resulting binaries will run on any macOS version above v10.15. Is it possible for us to do the same, and build the binaries on MacOS 13+, and allow hosts without xcode 15 installed to only install from a bottle?

@SMillerDev
Copy link
Member

Is it possible for us to do the same, and build the binaries on MacOS 13+, and allow hosts without xcode 15 installed to only install from a bottle?

No, Homebrew bottles are only build for the OS they are building on. But it would be good to ask upstream what the idea is. They might not know they broke older builds, so if we ask they can patch it.

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 Jul 22, 2024
@github-actions github-actions bot closed this Jul 29, 2024
@AndiDog
Copy link
Contributor

AndiDog commented Aug 9, 2024

For users wanting the latest tsh, for example because of ERROR: error reading from server: EOF:

brew remove teleport # v14.x at the moment, installs outdated tsh binary
brew install tsh # this is at v16.x already

@itsdalmo
Copy link
Author

itsdalmo commented Aug 9, 2024

Seems like the tsh formula downloads binaries that are built by Teleport? In that case it is subject to their new license, and might be in violation if one uses it for work in a company with >100 employees.

This was referenced Aug 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosquash Automatically squash pull request commits according to Homebrew style. CI-no-fail-fast Continue CI tests despite failing GitHub Actions matrix builds. go Go use is a significant feature of the PR or issue nodejs Node or npm use is a significant feature of the PR or issue stale No recent activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants