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

Don't immediately fail after a MacOS upgrade. #6591

Merged
merged 3 commits into from Oct 4, 2018

Conversation

Projects
None yet
5 participants
@benjyw
Copy link
Contributor

benjyw commented Oct 4, 2018

Currently if you upgrade MacOS to a version that
didn't exist at the time your Pants version was built,
Pants support binary fetching will fail. To fix this
we had to publish every support binary under a new path.

But this is unnecessary, since new versions of MacOS
can always run binaries built for older versions (at
least until Macs change their architecture again).

This change allows unknown MacOS versions to fall back
to the most recent version Pants knows about.

Don't immediately fail after a MacOS upgrade.
Currently if you upgrade MacOS to a version that
didn't exist at the time your Pants version was built,
Pants support binary fetching will fail. To fix this
we had to publish every support binary under a new path.

But this is unnecessary, since new versions of MacOS
can always run binaries built for older versions (at
least until Macs change their architecture again).

This change allows unknown MacOS versions to fall back
to the most recent version Pants knows about.

@benjyw benjyw force-pushed the benjyw:smooth_mac_upgrades branch from 4b229ee to affa043 Oct 4, 2018

@jsirois

jsirois approved these changes Oct 4, 2018

Copy link
Member

jsirois left a comment

LGTM mod small bug.

Show resolved Hide resolved src/python/pants/util/osutil.py Outdated
@stuhood

stuhood approved these changes Oct 4, 2018

Copy link
Member

stuhood left a comment

Thanks!

"""Return the (host, platform) pair for the highest known darwin version less than the bound."""
darwin_versions = [int(x[1]) for x in platform_name_map if x[0] == 'darwin']
max_darwin_version = str(max(v for v in darwin_versions if v <= int(darwin_version_upper_bound)))
return platform_name_map[('darwin', max_darwin_version)]

This comment has been minimized.

@stuhood

stuhood Oct 4, 2018

Member

Using get and raising a more helpful error here would be good (or having the caller handle None and fall through to the other helpful error you added).

This comment has been minimized.

@benjyw

benjyw Oct 4, 2018

Contributor

Well, assuming we have any darwin versions at all lower than the upper bound then this can never error, so i'm not sure what a helpful message could even say? And if we don't then max() will already have failed. I can check that this set isn't empty though.

This comment has been minimized.

@benjyw

benjyw Oct 4, 2018

Contributor

OK so this now falls through as you suggested.

@benjyw benjyw merged commit 3aae614 into pantsbuild:master Oct 4, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@benjyw benjyw deleted the benjyw:smooth_mac_upgrades branch Oct 4, 2018

@Eric-Arellano

This comment has been minimized.

Copy link
Contributor

Eric-Arellano commented Oct 23, 2018

I'm still getting this error when trying to setup Pants on macOS Mojave (10.14). Is this expected? I pulled the newest upstream.

$ ./pants clean-all
Downloading https://binaries.pantsbuild.org/bin/cmake/mac/10.14/3.9.5/cmake.tar.gz ...
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
curl: (22) The requested URL returned error: 404

Failed to build native engine.
@benjyw

This comment has been minimized.

Copy link
Contributor

benjyw commented Oct 24, 2018

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor

cosmicexplorer commented Oct 24, 2018

The above error is from the bootstrap script, which does not respect the override. The script could almost definitely be modified with a special case which checks for a 404 and tries e.g. the most recent supported version of MacOS instead (it may involve some changes to get_os.sh in addition to the bootstrap script, not sure).

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor

cosmicexplorer commented Oct 24, 2018

This could be done by modifying get_os.sh and download_binary.sh to do the above. I'll put up a PR in a second because this is blocking the other thing I'm trying to do.

@Eric-Arellano

This comment has been minimized.

Copy link
Contributor

Eric-Arellano commented Oct 24, 2018

@benjyw I have no custom config. I'm trying to install upstream Pants and everything is exactly the same as from GitHub.

@cosmicexplorer awesome, I'd be happy to be added to code review for that!

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor

cosmicexplorer commented Oct 25, 2018

I was interrupted for 12 straight hours but it's up now.

cosmicexplorer added a commit that referenced this pull request Oct 28, 2018

fall back to most recent known osx version for bootstrap binaries (#6681
)

### Problem

See the end of #6591 -- while we fall back to the most recent OSX version to download binaries for use with e.g. NativeTool, this fix does not apply for the binaries we fetch in shell scripts to bootstrap the pants rust code, leading to the following on OSX Mojave (10.14) in the pants repo:

```
Downloading https://binaries.pantsbuild.org/bin/cmake/mac/10.14/3.9.5/cmake.tar.gz ...
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
curl: (22) The requested URL returned error: 404
```

### Solution

- Append a `main()` method at the bottom of `binary_util.py` which can be invoked in the bootstrap phase by `download_binary.sh`.

### Result

I can run `./pants goals` on my OSX 10.14 laptop in the pants repo after an `rm -rf ~/.cache/pants`.

cosmicexplorer added a commit that referenced this pull request Nov 16, 2018

fall back to most recent known osx version for bootstrap binaries (#6681
)

### Problem

See the end of #6591 -- while we fall back to the most recent OSX version to download binaries for use with e.g. NativeTool, this fix does not apply for the binaries we fetch in shell scripts to bootstrap the pants rust code, leading to the following on OSX Mojave (10.14) in the pants repo:

```
Downloading https://binaries.pantsbuild.org/bin/cmake/mac/10.14/3.9.5/cmake.tar.gz ...
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
curl: (22) The requested URL returned error: 404
```

### Solution

- Append a `main()` method at the bottom of `binary_util.py` which can be invoked in the bootstrap phase by `download_binary.sh`.

### Result

I can run `./pants goals` on my OSX 10.14 laptop in the pants repo after an `rm -rf ~/.cache/pants`.

@caitp caitp referenced this pull request Nov 25, 2018

Closed

Can't build in OSX Mojave! #538

caitp added a commit to caitp/clusterfuzz-tools that referenced this pull request Nov 26, 2018

Upgrade pants to v1.9.0 to fix Mac builds
Originally, the intent was to update to 1.9.0 to include fixes from
pantsbuild/pants#6591, however the fixes aren't actually included in
1.9.0, and it may be necessary to use 1.11.0 or 1.12.0 to get the
full benefits.

In spite of this, at least locally, clusterfuzz-tools builds happily
under 1.9.0, so this may be enough to get things working under
MacOS Mojave.

Closes google#538

Dor1s added a commit to google/clusterfuzz-tools that referenced this pull request Nov 26, 2018

Upgrade pants to v1.9.0 to fix Mac builds (#539)
Originally, the intent was to update to 1.9.0 to include fixes from
pantsbuild/pants#6591, however the fixes aren't actually included in
1.9.0, and it may be necessary to use 1.11.0 or 1.12.0 to get the
full benefits.

In spite of this, at least locally, clusterfuzz-tools builds happily
under 1.9.0, so this may be enough to get things working under
MacOS Mojave.

Closes #538
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment