Skip to content

osx: fix osx binary backward compatibility #4396

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.

Sign up for GitHub

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

Jump to bottom
Merged
chakrabot merged 1 commit into from
Dec 12, 2017
Merged

osx: fix osx binary backward compatibility #4396

chakrabot merged 1 commit into from
Dec 12, 2017

Conversation

@obastemur
Copy link
Collaborator

@obastemur obastemur commented Dec 8, 2017

Fixes #4393

@obastemur
Copy link
Collaborator Author

obastemur commented Dec 8, 2017

Please help CR and merge.

@obastemur
Copy link
Collaborator Author

obastemur commented Dec 8, 2017

Release 1.7 branch is broken / failing on Windows with the error below;

07:54:32 3>    ch.exe    -DumpOnCrash -WERExceptionSupport -ExtendedErrorStackForTestHost -BaselineMode -forceNative -off:simpleJit -bgJitDelay:0 -dynamicprofileinput:profile.dpl.UnnamedTest62  -$262 D:\j\w\ci_dev12_x64_---276a6eb6\test\$262\$262test.js >D:\j\w\ci_dev12_x64_---276a6eb6\test\$262\testout2 2>&1
07:54:32 3>ERROR: name of output file: D:\j\w\ci_dev12_x64_---276a6eb6\test\$262\testout2; size: 152; creation: Fri Dec 08 07:45:55 2017, last access: Fri Dec 08 07:45:55 2017, now: Fri Dec 08 07:45:57 2017
07:54:32 3>ERROR: bad output file follows ============
07:54:32 3>test262/error Test262Error: Expected SameValue(«1ok», «0ok») to be true
07:54:32 3>test262/error Test262Error: Expected SameValue(«0ok», «1ok») to be true

This error is unrelated to what this PR does (xplat only). Any idea on this?

@MSLaguana
Copy link
Contributor

MSLaguana commented Dec 8, 2017

@dotnet-bot test Windows 7 ci_dev12_x64_debug please

MSLaguana
MSLaguana reviewed Dec 8, 2017
View reviewed changes
pal/src/misc/time.cpp Outdated
// The `__APPLE__` below is unlikely. However, if osx future version
// supports one of the clock types below, we may miss that and
// break backward compatibility.
#if !defined(__APPLE__) && HAVE_CLOCK_MONOTONIC_COARSE || HAVE_CLOCK_MONOTONIC
Copy link
Contributor

@MSLaguana MSLaguana Dec 8, 2017

Choose a reason for hiding this comment

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

Can you please put parens around this like #if !define (__APPLE__) && (HAVE_CLOCK_MONOTONIC_COARSE || HAVE_CLOCK_MONOTONIC) because I can never remember which boolean operators have stronger precedence.

Copy link
Collaborator Author

@obastemur obastemur Dec 8, 2017

Choose a reason for hiding this comment

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

👍

@obastemur
Copy link
Collaborator Author

obastemur commented Dec 8, 2017

@MSLaguana do we track that issue?

@MSLaguana
Copy link
Contributor

MSLaguana commented Dec 8, 2017

This is the first I heard about that test being flaky, but considering it succeeded on re-run it seems that's the case.

@obastemur
Copy link
Collaborator Author

obastemur commented Dec 9, 2017

@MSLaguana updated the PR, thanks for the review.

@chakrabot chakrabot merged commit 9368f90 into chakra-core:release/1.7 Dec 12, 2017
chakrabot pushed a commit that referenced this pull request Dec 12, 2017
@MSLaguana
[MERGE #4396 @obastemur] osx: fix osx binary backward compatibility
3e73938 
Merge pull request #4396 from obastemur:fix_build_osx_bb1

Fixes #4393
chakrabot pushed a commit that referenced this pull request Dec 12, 2017
@MSLaguana
[1.7>1.8] [MERGE #4396 @obastemur] osx: fix osx binary backward compa…
39d16c3 
…tibility

Merge pull request #4396 from obastemur:fix_build_osx_bb1

Fixes #4393
chakrabot pushed a commit that referenced this pull request Dec 12, 2017
@MSLaguana
[1.8>master] [1.7>1.8] [MERGE #4396 @obastemur] osx: fix osx binary b…
97fdeb8 
…ackward compatibility

Merge pull request #4396 from obastemur:fix_build_osx_bb1

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

Reviewers

@bterlson bterlson Awaiting requested review from bterlson

@digitalinfinity digitalinfinity Awaiting requested review from digitalinfinity

@jianchun jianchun Awaiting requested review from jianchun

1 more reviewer

@MSLaguana MSLaguana MSLaguana approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@obastemur @MSLaguana @chakrabot