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

swiftlint 0.42.0 #66918

Closed
wants to merge 4 commits into from
Closed

swiftlint 0.42.0 #66918

wants to merge 4 commits into from

Conversation

jpsim
Copy link
Contributor

@jpsim jpsim commented Dec 14, 2020

Created with brew bump-formula-pr.

@jpsim
Copy link
Contributor Author

jpsim commented Dec 15, 2020

Is it normal for the CI checks to still be pending 6 hours later?

@chenrui333
Copy link
Member

chenrui333 commented Dec 15, 2020

it is not normal but we only have limited resources for all CI jobs.

@norio-nomura
Copy link
Contributor

Apparently, Homebrew hijacks the clang call and adds an option -march=nehalem that we don't want, so it fails to build with following error:

clang: error: the clang compiler does not support '-march=nehalem'

This error can be avoided by making the following changes and overriding the variables that Homebrew's clang receives:

--- a/Formula/swiftlint.rb
+++ b/Formula/swiftlint.rb
@@ -18,7 +18,7 @@ class Swiftlint < Formula
   depends_on xcode: "8.0"
 
   def install
-    system "make", "prefix_install", "PREFIX=#{prefix}", "TEMPORARY_FOLDER=#{buildpath}/SwiftLint.dst"
+    system "env", "HOMEBREW_OPTFLAGS=", "make", "prefix_install", "PREFIX=#{prefix}", "TEMPORARY_FOLDER=#{buildpath}/SwiftLint.dst"
   end
 
   test do

@SMillerDev
Copy link
Member

That really shouldn't happen. Why doesn't the compiler accept that argument?

@norio-nomura
Copy link
Contributor

make prefix_install uses the command swift build --disable-sandbox --configuration release --arch arm64 --arch x86_64 to generate the universal binary when building swiftlint.
I think it's because -march=nehalem is passed to clang regardless of --arch, which causes an error when generating the object file for arm64.

@SMillerDev
Copy link
Member

We don't want to ship universal binaries so that should be stripped out either way.

@jpsim
Copy link
Contributor Author

jpsim commented Dec 15, 2020

We don't want to ship universal binaries so that should be stripped out either way.

Could you please elaborate why you don't want to ship universal binaries? It seems that with many individuals running Homebrew on M1 Macs via Rosetta 2 (either with arch -x86_64 or by forcing Terminal to run in x86 mode) it would be preferable to install the universal binary in those cases so that they always run the platform native version of SwiftLint.

In our case, the universal binary isn't particularly large so the benefits of a universal binary outweigh the costs.

@SMillerDev
Copy link
Member

Could you please elaborate why you don't want to ship universal binaries? It seems that with many individuals running Homebrew on M1 Macs via Rosetta 2 (either with arch -x86_64 or by forcing Terminal to run in x86 mode) it would be preferable to install the universal binary in those cases so that they always run the platform native version of SwiftLint.

Because we build bottles for a specific architecture. It conflicts with the brew settings if both Big Sur bottles would install a fat binary.

On top of that, we had a lot of issues with the last time universal binaries were a thing and we did support them. We'd like to avoid that this time around and spend our time building specifically for the configurations that users are requesting.

@jpsim
Copy link
Contributor Author

jpsim commented Dec 15, 2020

Got it. I made some edits to the formula installation, let's see if that works.

@BrewTestBot BrewTestBot added the swift Swift use is a significant feature of the PR or issue label Dec 15, 2020
@jpsim
Copy link
Contributor Author

jpsim commented Dec 15, 2020

For posterity, I had to pass --disable-sandbox to swift build to work around this issue:

$ swift build --configuration release -Xswiftc -static-stdlib
2020-12-15T18:13:30.6435360Z /private/tmp/swiftlint-20201215-87068-1qjxucs: error: manifest parse error(s):
2020-12-15T18:13:30.6438130Z sandbox-exec: sandbox_apply: Operation not permitted

I believe this happens because the Swift Package Manager doesn't normally build in a macOS sandboxed environment.

Specifying --disable-sandbox disables using the sandbox when executing subprocesses: https://github.com/apple/swift-package-manager/blob/a8ded58a5ec233f8b4be294762cae2c821081a1e/Sources/Commands/Options.swift#L168-L170

@SMillerDev
Copy link
Member

Thanks @jpsim ! Without contributions like yours it'd be impossible to keep homebrew going with the high standards that users have come to expect from the project. You can feel good knowing that you've made the world a tiny bit better for homebrew users around the world! 👍 🎉

@BrewTestBot
Copy link
Member

🤖 A scheduled task has triggered a merge.

@jpsim jpsim deleted the bump-swiftlint-0.42.0 branch December 16, 2020 13:28
@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Jan 16, 2021
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Jan 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age swift Swift use is a significant feature of the PR or issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants