Skip to content

Upgrade macOS/Travis default image to xcode6.4#6

Merged
jakirkham merged 12 commits intoconda-forge:masterfrom
inducer:master
Jan 10, 2017
Merged

Upgrade macOS/Travis default image to xcode6.4#6
jakirkham merged 12 commits intoconda-forge:masterfrom
inducer:master

Conversation

@inducer
Copy link
Contributor

@inducer inducer commented Nov 15, 2016

Here's a quick straw man for a CFEP to get the Travis image changed.

cc @jakirkham @conda-forge/core

@inducer
Copy link
Contributor Author

inducer commented Nov 15, 2016

Pretty version

cfep-02.md Outdated
## Rationale

* The current default image (`beta-xcode6.1`) is deprecated
and will cease to be supported/provided by Travis [@jakirkham: citation needed],
Copy link

Choose a reason for hiding this comment

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

@mwcraig
Copy link

mwcraig commented Nov 15, 2016

This seems inevitable to me. One reasonable approach to testing (not saying I actually have time to implement it, unfortunately) might be to build a couple test packages on xcode 6.4 that target 10.9, and use the xcode 6.1 while it is still around to try running test suites for those packages.

That won't catch everything, but at some point we are really constrained in what we can do by what the CI services make it easy to do.

@inducer
Copy link
Contributor Author

inducer commented Nov 15, 2016

Matt Craig notifications@github.com writes:

@inducer -- here is the ref: travis-ci/travis-ci#6765 (comment)

Thanks, updated!

* [Travis image support policy][travis]

[impl]: https://github.com/conda-forge/conda-forge.github.io/issues/249#issuecomment-256207392
[travis]: https://github.com/travis-ci/travis-ci/issues/6765#issuecomment-256703076
Copy link
Member

Choose a reason for hiding this comment

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

cfep-02.md Outdated
for that image) by hand.

* Even with this change, this would implicitly drop support for
macOS 10.7 and 10.8.
Copy link
Member

Choose a reason for hiding this comment

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

We never have had support for these as we were always using the beta-xcode6.1 image, which only has the 10.9 and 10.10 SDKs.

@inducer
Copy link
Contributor Author

inducer commented Nov 15, 2016

jakirkham notifications@github.com writes:

Other relevant references.

Added, thx!

cfep-02.md Outdated

* We won't be able to compile [clang
3.8/3.9](https://github.com/conda-forge/staged-recipes/pull/1481) without
this change.
Copy link
Member

Choose a reason for hiding this comment

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

Also in need of this image are

```

* Adapt `conda smithy` so that it applies that edit on rerender.
* Apply the change in the `staged-recipes` master branch.
Copy link
Member

Choose a reason for hiding this comment

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

Would link this PR ( conda-forge/staged-recipes#1094 ).

osx_image: xcode6.4
```

* Adapt `conda smithy` so that it applies that edit on rerender.
Copy link
Member

Choose a reason for hiding this comment

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

Simply need to change this line. So also easy. 😄

cfep-02.md Outdated

## Backward Compatibility

* The default deployment target for `xcode6.4` is macOS 10.11.
Copy link
Member

Choose a reason for hiding this comment

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

Should be 10.10 as it doesn't have the 10.11 SDK FWICT.


* The current default image (`beta-xcode6.1`) is deprecated
and will cease to be [supported/provided by Travis][travis],
so this change will have to happen at some point anyway.
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 until January 20th, 2017. Would actually include that date verbatim as I don't think many people know this.

Copy link
Member

Choose a reason for hiding this comment

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

Noted below.

@inducer
Copy link
Contributor Author

inducer commented Nov 15, 2016

@jakirkham Please feel free to push directly to the text. Added you to the repo.


* Adapt `conda smithy` so that it applies that edit on rerender.
* Apply the change in the `staged-recipes` master branch.
* Rebuild all packages with this? (TBD? Opinions?)
Copy link
Member

Choose a reason for hiding this comment

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

Don't think this is required. However, the most recent document I can find on C++ ABI compatibility is discussing 10.3.8 and before vs. 10.3.9 and after. Those OSes (and their associated compilers) are so ancient this seems irrelevant, but have included the link anyways.

xref: https://developer.apple.com/library/content/documentation/DeveloperTools/Conceptual/CppRuntimeEnv/Articles/CPPROverview.html

@jakirkham
Copy link
Member

Thanks for writing all of this up, @inducer ! This is very helpful. Hopefully this will help get this important discussion moving.

@jakirkham
Copy link
Member

Would also appreciate your feedback on this.

cc @izaid @insertinterestingnamehere @wesm

@inducer
Copy link
Contributor Author

inducer commented Nov 15, 2016

@jakirkham Thanks for your comments. I think I've got most of the massaged into the text.

@wesm
Copy link
Member

wesm commented Nov 16, 2016

+1

* [@jakirkham on implications][impl]
* [Travis image support policy][travis]
* [Travis CI changes default to Mac OS 10.11 XCode 7.3]( https://blog.travis-ci.com/2016-10-04-osx-73-default-image-live/ )
* [Travis CI image restructuring]( https://blog.travis-ci.com/2016-09-15-new-default-osx-image-coming/ )
Copy link
Member

Choose a reason for hiding this comment

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

Probably should add this recent one too.

xref: https://blog.travis-ci.com/2016-11-17-retiring-some-osx-images/

@inducer
Copy link
Contributor Author

inducer commented Nov 21, 2016

jakirkham notifications@github.com writes:

Probably should add this recent one too.

xref: https://blog.travis-ci.com/2016-11-17-retiring-some-osx-images/

Done.

@jakirkham
Copy link
Member

Also brought this up at an informal meeting last week. It should come up in the formal meeting this week. I'd recommend anyone interested in this issue to go if they can. Unfortunately it falls on Thanksgiving US. So if people are unable to make it, I completely understand. Sadly I know I won't be able to make it either. Details linked below.

xref: https://conda-forge.hackpad.com/conda-forge-meetings-2YkV96cvxPG#:h=2016-11-24:-General-Discussion

@minrk
Copy link
Member

minrk commented Nov 25, 2016

I've started seeing this message on Travis:

screen shot 2016-11-25 at 12 47 53

which suggests that time is of the essence, and Travis' previous messaging about 6.1 being safe until January might no longer be accurate.

I also opened conda-forge/conda-smithy#391, sibling to conda-forge/staged-recipes#1094, which can be merged once this is agreed upon.

And based on the info linked here, I wrote this script for inspecting the deployment target of dylibs, inspired a bit by conda inspect linkages. We can try adding it somewhere so that it can be called during the test phase on Travis, but I'm not 100% sure where the right place would be to open a PR (smithy?, toolchain? conda-build?).


* The default deployment target for `xcode6.4` is macOS 10.10.
It would need to be forced to 10.9 (the lowest supported one
for that image) by hand.
Copy link
Member

Choose a reason for hiding this comment

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

"by hand" is perhaps an overstatement - the toolchain package (mentioned above) already does this, and staged-recipes already includes a note in the build section that any package that compiles things should pull in toolchain as a build dependency.

Copy link
Member

Choose a reason for hiding this comment

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

After some discussion at our meeting yesterday, we have added PR ( conda-forge/conda-forge-build-setup-feedstock#46 ), which forces the minimum build target on OS X to 10.9. While personally I still recommend the toolchain package and it does have broad usage throughout conda-forge, having the deployment target set in conda-forge-build-setup guarantees that everything builds in a compatible way. This should help further reduce the burden on feedstock maintainers by setting compatibility at the beginning of each build.

@insertinterestingnamehere
Copy link
Member

OSX development isn't my primary area of expertise, but FWIW I think this is a great idea.

@jakirkham
Copy link
Member

Also have been seeing that @minrk . It is unclear to me whether this is a warning that they added to all the images being deprecated (of which there are several) or if it has meaning in this case. I think we still get the image until January. However, the date it drops has become a bit fuzzy.

xref: travis-ci/travis-ci#6765 (comment)

@jakirkham
Copy link
Member

To help ease some concerns, the Travis CI beta-xcode6.1 image will remain until January 20th.

xref: travis-ci/travis-ci#6765 (comment)

cc @minrk

@minrk
Copy link
Member

minrk commented Nov 28, 2016

@jakirkham thanks for checking!

I think this should be a pretty safe upgrade to make. Any recipe that pulls in toolchain should already be set. The main thing to do is to ensure that, as packages are updated, make sure toolchain is pulled in as a dependency for those that don't have this already. We can do this at the human level (easiest, highest chance of occasional broken package on 10.9-only), or wait until a verification script like the one I linked above can be run automatically on build on OS X. I'm happy to start a PR for that, but I'm not sure yet which is the right repo to do it.

@inducer
Copy link
Contributor Author

inducer commented Dec 5, 2016

What's the criterion for officially calling this 'accepted'? Is there an official voting process? IMO, this is mostly a no-brainer, since the choice is mainly between "Do we wait a month until Travis forces our hand, or do we do this step-by-step on our own terms?'

Copy link
Member

@pelson pelson left a comment

Choose a reason for hiding this comment

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

This looks great to me @inducer. Thanks for taking the time to write this down. We still lack governance on "consensus", but given there is a cliff edge approaching, we need to time-bound the process so that we have a decision in the next few weeks (i.e. before 2017) so that we can start to implement/percolate the change across conda-forge.

The huge unknown is still


* Rebuild all packages with this? (TBD? Opinions?)

Likely not necessary given C++ ABI Compatibility.

If we can resolve this one piece of information definitively, we will know roughly how much work this whole proposal will entail (and indeed, how much work is lurking for us if we do drop off the cliff on the 20th Jan).

cfep-02.md Outdated
* Rebuild all packages with this? (TBD? Opinions?)

Likely not necessary given [C++ ABI Compatibility][cxxabi].
* Suggest that all new packages include the `toolchain` so that
Copy link
Member

Choose a reason for hiding this comment

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

@ocefpaf - I'd appreciate your input on this point.

Copy link
Member

Choose a reason for hiding this comment

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

I am against the use of any un-versioned package that can change the build characteristics like that. We already have have feedstocks that have to override those variables to build properly. IMO those belong in the recipe and are, ultimately, the maintainers decision.

With that said I no longer have a stake on this as we diverged from conda-forge a long time ago, since the toolchain was first added. Unfortunately most of earth-science software we build locally must use gcc and must provide libgomp and others at run time to ensure they will run anywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So what do you suggest? Would you prefer a versioned dependency on toolchain? (And isn't each maintainer still at liberty to decide whether they want a versioned or unversioned dependency?) Or should each maintainer figure out setting things like the build target by hand? (And who catches it if someone forgets?)

Copy link
Member

Choose a reason for hiding this comment

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

`> So what do you suggest?

Recommend but do not enforce. Many add the toolchain without even knowing what is the impact of those flags to the builds.

And who catches it if someone forgets?

Correct me if I am wrong but the build target is not too bad if an old one is used, right?

Anyways, like I said above, I no longer have a stake on this and I don't really care what is decided here. I am following @pelson's own workflow to re-build everything for my day job. Also, many of the users that had issues with the toolchain, that broke the GIS stack, already left conda-forge.

Copy link
Member

Choose a reason for hiding this comment

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

I still advocate the toolchain and it is versioned. We can pin the versions in the feedstocks. We can even add the pinned version to the pinning script. That all being said, I feel like Filipe and I have had this exact conversation over and over without a different result. So am not eager to dive into it again.

Instead I would like to find a reasonable middle ground that everyone can agree on and is easy to implement. As we do actually need to set the Mac OS X deployment target. My proposal would be to set the MACOSX_DEPLOYMENT_TARGET in conda-forge-build-setup as show in PR ( conda-forge/conda-forge-build-setup-feedstock#46 ). @msarahan helped me with a PR to whitelist the MACOSX_DEPLOYMENT_TARGET in conda-build. So this will work with conda-build 2.0.12+.

I think this proposal is nice for a few reasons. First it sets the deployment target everywhere immediately. So we don't have to worry about people who are unaware of this change accidentally breaking compatibility. Second it consolidates the effort between upgrading to conda-build 2 and upgrading the Travis CI image. We need conda-build 2 to leverage this feature. So the two efforts should not be "competing" for time, but working together. Third it moves the discussion away from compiler preferences to maintaining compatibility of binaries. As compatibility is the real concern we are dealing with here, it makes sense to make that the focus and this seems to the easiest most straightforward way to do that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 I've removed the toolchain wording and replaced it by a summary of what you've suggested.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks @inducer .

in packages that do not meet conda-forge's promise of OS compatibility
back to 10.9.

* Multiple packages need this to move forward:
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a one-sentence context that summarises the need?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@wesm
Copy link
Member

wesm commented Dec 30, 2016

What's our timeline to resolve this? January 21 is only a few weeks away

@jakirkham
Copy link
Member

I put it on the meeting agenda.

cfep-02.md Outdated

<table>
<tr><td> Title </td><td> Upgrade default macOS Travis image to `xcode6.4` </td>
<tr><td> Status </td><td> Draft </td></tr>
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to change the status?

Copy link
Member

@jakirkham jakirkham left a comment

Choose a reason for hiding this comment

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

LGTM

Not sure if we need to change the status to "Accepted", but this looks like a complete proposal ready to merge.

@jakirkham
Copy link
Member

Thanks for your work on this, @inducer .

@wesm
Copy link
Member

wesm commented Jan 7, 2017

Thanks everyone.

@inducer
Copy link
Contributor Author

inducer commented Jan 7, 2017

Updated the header to reflect acceptance.

@jakirkham
Copy link
Member

Any final thoughts, @conda-forge/core ?

@mwcraig
Copy link

mwcraig commented Jan 10, 2017

Looks great!

@jakirkham
Copy link
Member

Alright, given as we have 10 days to get this sorted and I'm not hearing any complaints about this revision, am going to go ahead and merge it. Though if there are corrections, please feel free to raise issues and PRs.

@jakirkham jakirkham merged commit 0610308 into conda-forge:master Jan 10, 2017
@jakirkham
Copy link
Member

Thanks @inducer for putting this together and thanks everyone for the thorough review. 👍

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants