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

mas 1.4.2 #31058

Closed
wants to merge 1 commit into from
Closed

mas 1.4.2 #31058

wants to merge 1 commit into from

Conversation

phatblat
Copy link
Contributor

  • Have you followed the guidelines for contributing?
  • 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?
  • Does your build pass brew audit --strict <formula> (after doing brew install <formula>)?

@DomT4
Copy link
Member

DomT4 commented Aug 13, 2018

You got a lot further than I did, heh. Current failing here:

<unknown>:0: error: module map file '/tmp/mas-20180813-91738-18nl0xo/mas-1.4.2/build/Release/Commandant/Commandant.modulemap' not found
15 errors generated.
<unknown>:0: error: failed to emit precompiled header '/Users/brew/Library/Developer/Xcode/DerivedData/mas-cli-fizohytgcqxfgdbaqogeyqrzydbc/Build/Intermediates.noindex/PrecompiledHeaders/mas-cli-Bridging-Header-swift_3FGJUF6TZSKJ6-clang_3V470TLVS6FZA.pch' for bridging header '/tmp/mas-20180813-91738-18nl0xo/mas-1.4.2/App/mas-cli-Bridging-Header.h'

** BUILD FAILED **


The following build commands failed:
	CompileSwift normal x86_64
	CompileSwiftSources normal x86_64 com.apple.xcode.tools.swift.compiler
(2 failures)

@phatblat
Copy link
Contributor Author

Strange, I'll investigate.

@DomT4 DomT4 added the build failure CI fails while building the software label Aug 14, 2018
@phatblat
Copy link
Contributor Author

phatblat commented Aug 15, 2018

Found the issue. Using a relative path for SRCROOT SYMROOT is breaking things since there are two Xcode projects at different folder levels.

It looks like buildpath should be an absolute path to where the formula is built. I tried using that as the base for SRCROOT SYMROOT but am still seeing the same error (I may be doing something wrong). Is there another way to get an absolute path?

@DomT4
Copy link
Member

DomT4 commented Aug 15, 2018

If you can set SRCROOT try setting it to buildpath.realpath.

@phatblat
Copy link
Contributor Author

Think I've got it now. Ended up using File.absolute_path(buildpath) to get it.

@phatblat
Copy link
Contributor Author

Failed on sierra

20:43:55 ==> gem install bundler
20:43:55 ERROR:  Could not find a valid gem 'bundler' (>= 0), here is why:
20:43:55           Unable to download data from https://rubygems.org/ - SSL_connect returned=1 errno=0 state=SSLv2/v3 read server hello A: tlsv1 alert protocol version (https://rubygems.org/latest_specs.4.8.gz)

@DomT4
Copy link
Member

DomT4 commented Aug 16, 2018

Ended up using File.absolute_path(buildpath) to get it.

Did buildpath.realpath not work in the end?

Failed on sierra

depends_on "ruby" => :build if MacOS.version <= :sierra will fix that.

Formula/mas.rb Outdated
@@ -14,11 +14,34 @@ class Mas < Formula
depends_on :xcode => ["9.0", :build]

def install
xcodebuild "-project", "mas-cli.xcodeproj",
# Pre-install a shallow copy of the CocoaPods master repo
system "git", "clone", "--depth", "1", "https://github.com/CocoaPods/Specs.git", File.expand_path("~/.cocoapods/repos/master")
Copy link
Member

Choose a reason for hiding this comment

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

Can we add this as a resource and then stage the resource instead of using git clone directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good idea. 💡

@phatblat
Copy link
Contributor Author

I think our custom tap may have been shadowing the core formula when I was testing buildpath.realpath. Let me try that again.

Formula/mas.rb Outdated
bash_completion.install "contrib/completion/mas-completion.bash" => "mas"
end

def caveats; <<~EOS
Copy link
Member

Choose a reason for hiding this comment

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

I'd rather not include these caveats, personally, as much as I appreciate that may be frustrating for y'all. We try to limit caveats pretty hard because, well, nobody really reads them and it doesn't help there are so many already 🙃.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

gotcha, I'll 🔥 it

@phatblat
Copy link
Contributor Author

Yeah, buildpath.realpath worked: SYMROOT=/private/tmp/mas-20180815-81376-1d0ucqx/mas-1.4.2

@DomT4
Copy link
Member

DomT4 commented Aug 17, 2018

New failure on Sierra:

00:49:48 /tmp/mas-20180817-89895-wnikyq/mas-1.4.2/App/Commands/Install.swift:19:39: error: value of type '[UInt64]' has no member 'compactMap'
00:49:48         let downloadResults = options.appIds.compactMap { (appId) -> MASError? in
00:49:48                               ~~~~~~~~^~~~~~ ~~~~~~~~~~
00:49:48 /tmp/mas-20180817-89895-wnikyq/mas-1.4.2/App/Commands/Lucky.swift:38:31: error: value of type '[UInt64]' has no member 'compactMap'
00:49:48         let downloadResults = [appId].compactMap { (appId) -> MASError? in
00:49:48                               ^~~~~~~ ~~~~~~~~~~
00:49:48 /tmp/mas-20180817-89895-wnikyq/mas-1.4.2/App/Commands/Upgrade.swift:29:22: error: value of type '[String]' has no member 'compactMap'
00:49:48             appIds = apps.compactMap {
00:49:48                      ^~~~ ~~~~~~~~~~
00:49:48 /tmp/mas-20180817-89895-wnikyq/mas-1.4.2/App/Commands/Upgrade.swift:41:23: error: value of type '[UInt64]' has no member 'compactMap'
00:49:48             updates = appIds.compactMap {
00:49:48                       ^~~~~~ ~~~~~~~~~~
00:49:48 /tmp/mas-20180817-89895-wnikyq/mas-1.4.2/App/Commands/Upgrade.swift:62:29: error: value of type '[CKUpdate]' has no member 'compactMap'
00:49:48         let updateResults = updates.compactMap {
00:49:48                             ^~~~~~~ ~~~~~~~~~~
00:49:48 
00:49:48 ** BUILD FAILED **
00:49:48 
00:49:48 
00:49:48 The following build commands failed:
00:49:48 	CompileSwift normal x86_64
00:49:48 	CompileSwiftSources normal x86_64 com.apple.xcode.tools.swift.compiler
00:49:48 (2 failures)
00:49:48 

@phatblat
Copy link
Contributor Author

Oh joy. Didn't realize that by upgrading to Swift 4.1 that it requires Xcode 9.3 and drops support for sierra.

@DomT4
Copy link
Member

DomT4 commented Aug 17, 2018

Ah, yeah, that'll do it. Apple is fairly sharp on dropping support for older Xcode versions, for better or worse.

I think the discussion around system "git", "clone", "--depth", "1", "https://github.com/CocoaPods/Specs.git" is the last thing that needs doing here, assuming that CI comes back fine this time, which seems likely based off the last run.

@phatblat
Copy link
Contributor Author

Yep. Just working on a script to capture a snapshot of that git repo.

@DomT4
Copy link
Member

DomT4 commented Aug 17, 2018

In Homebrew terms you should be able to do something like:

resource "cocoapods" do
  url "https://github.com/CocoaPods/Specs.git",
  :revision => "ce31522ca4adfa4b4bd07dfc3bc10bbd1dd495d7"
end

def install
  (buildpath/".brew_home/.cocoapods/repos/master").install resource("cocoapods")
end

@phatblat
Copy link
Contributor Author

Tried 3 approaches for this resource since I wasn't happy with the build times due to this huge git repo.

  1. Cloning the full CocoaPods/Specs repo as per your example
    • 2.3 GB
    • 23m 🐌
  2. Download a shallow clone of the repo
    • 124 MB
    • 7m 🐢
  3. Download a minimal copy containing only the pods used by this project
    • 22 KB
    • 44s 🚗 💨

Thanks for all your help @DomT4!

@DomT4
Copy link
Member

DomT4 commented Aug 18, 2018

Cloning the full CocoaPods/Specs repo as per your example

I thought resources were supposed to be shallow from GitHub (and other whitelisted links) unless explicitly requested otherwise. Am I forgetting something on this @reitermarkus?

Download a minimal copy containing only the pods used by this project

Is there a way to do this without relying on you/someone else upstream always being the one to update mas? I think the current method here pretty drastically reduces the formula's 🚌 factor, which worries me a little.

@phatblat
Copy link
Contributor Author

This new complexity is the result of switching to using CocoaPods to manage dependencies in mas-cli/mas#154. I'm having second thoughts because - as you mention - it's made maintaining the formula harder. Debating using Carthage instead.

If it's any consolation, I view installing mas through Homebrew as our primary means of distribution. We have our own custom tap to support older macOS versions since the recent changes to use newer versions of Swift and Xcode have dropped support for building on all but the current version ☹️ . Also, I'm currently working on adding Homebrew building to our CI process even for PRs (mas-cli/mas#156)

@phatblat
Copy link
Contributor Author

phatblat commented Sep 2, 2018

@DomT4 can we move this along?

@SConaway
Copy link
Contributor

SConaway commented Sep 5, 2018

@phatblat @DomT4 what's going on here?

@phatblat
Copy link
Contributor Author

phatblat commented Sep 5, 2018

Lots of changes to the mas formula were necessary in order to get it to build within the Homebrew environment due to the addition of CocoaPods to manage dependencies. Part of the complexity was CocoaPods downloads a huge repo. Where it stands now is a bit of a hack to prevent that download and use a slimmed-down pod repo.

I'm hoping we can move forward with this as-is and address maintenance concerns with a future update. mas-cli/mas#169 already addresses this, but I need to review it and do some testing.

@SConaway
Copy link
Contributor

SConaway commented Sep 5, 2018 via email

depends_on :xcode => ["9.3", :build]

resource "cocoapods" do
url "https://dl.bintray.com/phatblat/mas-bottles/master.tar.gz"
Copy link
Member

@fxcoudert fxcoudert Sep 6, 2018

Choose a reason for hiding this comment

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

I don't think downloading an unversioned, non-official copy of the repo is acceptable in the Homebrew way of handling dependencies.

What are the alternatives? We have a cocoapods formula which we could depend on, couldn't we?

Copy link
Member

Choose a reason for hiding this comment

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

We have a cocoapods formula which we could depend on, couldn't we?

The cocoapods formula installs the command, not the repo.

Copy link
Member

Choose a reason for hiding this comment

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

Understood. I think the only option is thus to wait for mas-cli/mas#169 to be merged, which moves away from cocoapods and will allow a build compatible with Homebrew's rules.

Copy link
Contributor

Choose a reason for hiding this comment

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

Wasn't this suggestion #31058 (comment) using the official repo ?

@javian javian changed the title mas (1.4.2) mas 1.4.2 Sep 8, 2018
depends_on :xcode => ["9.3", :build]

resource "cocoapods" do
url "https://dl.bintray.com/phatblat/mas-bottles/master.tar.gz"
Copy link
Member

Choose a reason for hiding this comment

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

The shallow clone approach seems like the only one workable.

@stale
Copy link

stale bot commented Sep 30, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the stale No recent activity label Sep 30, 2018
@phatblat
Copy link
Contributor Author

phatblat commented Oct 2, 2018

Closing. Will redo formula for mas 1.4.3 after mas-cli/mas#169

@phatblat phatblat closed this Oct 2, 2018
@jeroenj jeroenj mentioned this pull request Oct 4, 2018
4 tasks
@phatblat phatblat mentioned this pull request Oct 18, 2018
4 tasks
@lembacon lembacon mentioned this pull request Oct 18, 2018
4 tasks
@lock lock bot added the outdated PR was locked due to age label Nov 1, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Nov 1, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
build failure CI fails while building the software outdated PR was locked due to age stale No recent activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants