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

Guard unwind test against old OS X versions #476

Merged
merged 1 commit into from Feb 23, 2016
Merged

Conversation

btj
Copy link
Contributor

@btj btj commented Feb 18, 2016

tests/unwind crashes on OS X 10.8. This fixes that by only running the test if a sufficiently new version of ld is present on the system.

gasche added a commit that referenced this pull request Feb 23, 2016
Guard unwind test against old OS X versions
@gasche gasche merged commit 76d0fbe into ocaml:trunk Feb 23, 2016
@gasche
Copy link
Member

gasche commented Feb 23, 2016

I merged in trunk ( 17e5435 ) and in 4.03 ( 7a16f2f ).

@gasche
Copy link
Member

gasche commented Feb 23, 2016

So this patch breaks our CI infrastructure because the bash-only matching is not supported everywhere.

I tried to rewrite it in a more portable way in dd5dc84 , which I pushed in trunk to get some feedback from the CI infrastructure. @btj , would you mind looking at it to confirm that it has the intended semantics?

@damiendoligez : apoligies for the hasty merge in the release branch. In the future I'll merge in trunk first and wait for CI report, even for stuff that we want in 4.03 and "obviously doesn't break anything". I can revert the commit if you think it's best, but I hope that the trunk CI will look good and my plan is to merge dd5dc84 directly.

@gasche
Copy link
Member

gasche commented Feb 23, 2016

The patch seems to fix the CI on non-mac machines, so I also pushed it into 4.03 to help people currently testing the branch: 1d9c4a0 . I would still be interested in a confirmation that it also solves the original issue on OSX 10.8 (this CI machine is disabled currently).

@btj
Copy link
Contributor Author

btj commented Feb 23, 2016

I don't think this does the right thing: the output of ld -v 2>&1 is as follows on my machine:

@(#)PROGRAM:ld  PROJECT:ld64-241.9
configured to support archs: armv6 armv7 armv7s arm64 i386 x86_64 x86_64h armv6m armv7m armv7em
LTO support using: LLVM version 3.5svn

Notice that 1) the version does not appear at the start of the line, and 2) the output consists of 3 lines.

@gasche
Copy link
Member

gasche commented Feb 23, 2016

@btj thanks for the quick reaction; would 1d6e092 (in my local fork) do the job?

(I tested with your ld output and it seemed to work.)

@btj
Copy link
Contributor Author

btj commented Feb 23, 2016

Yes, except that you do need the 2>&1: ld -v writes to stderr, not stdout.

@gasche
Copy link
Member

gasche commented Feb 23, 2016

Of course; I used a literal string for testing, then put back the wrong thing, sorry. The fixed commit is 8dcb687 , and I'm going to sit on it for a dozen of minutes to make sure I haven't forgotten about something, and then push it to 4.03 and trunk.

@btj
Copy link
Contributor Author

btj commented Feb 23, 2016

This works on my machine (OS X 10.10.1).

@gasche
Copy link
Member

gasche commented Feb 23, 2016

I pushed them ( 8dcb687 in trunk; 6151d4c in 4.03 ). Thanks!

@damiendoligez
Copy link
Member

@gasche

@damiendoligez : apoligies for the hasty merge in the release branch. In the future I'll merge in trunk first and wait for CI report, even for stuff that we want in 4.03 and "obviously doesn't break anything".

No need to apologize, I've done my fair share of "obviously doesn't break anything" myself...

Keryan-dev added a commit to Keryan-dev/ocaml that referenced this pull request Jun 21, 2021
Gbury pushed a commit to Gbury/ocaml that referenced this pull request Jun 22, 2021
chambart pushed a commit to chambart/ocaml-1 that referenced this pull request Feb 1, 2022
EmileTrotignon pushed a commit to EmileTrotignon/ocaml that referenced this pull request Jan 12, 2024
Co-authored-by: Thibaut Mattio <thibaut.mattio@gmail.com>
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.

None yet

3 participants