Skip to content

[WiP] Remove <experimental/optional> hack from util/optional.h#2640

Closed
uklotzde wants to merge 2 commits into
mixxxdj:masterfrom
uklotzde:util_optional
Closed

[WiP] Remove <experimental/optional> hack from util/optional.h#2640
uklotzde wants to merge 2 commits into
mixxxdj:masterfrom
uklotzde:util_optional

Conversation

@uklotzde
Copy link
Copy Markdown
Contributor

@uklotzde uklotzde commented Apr 9, 2020

Try to fix the macOS build on the build server caused by #2601.

@uklotzde uklotzde requested review from Be-ing and daschuer April 9, 2020 17:14
@daschuer
Copy link
Copy Markdown
Member

daschuer commented Apr 9, 2020

I am afraid the optional header is actually not available and this will fail anyway.

If this is not the case this docs here would be void which is very unlikely.
https://libcxx.llvm.org/docs/FeatureTestMacroTable.html
I clearly state that our conditional should work properly.

Can we have a look on the server and verify this before merge?

I think a very easy fix would be to ignore the error in the experimental case.
-Wno-invalid-constexpr

@Be-ing
Copy link
Copy Markdown
Contributor

Be-ing commented Apr 9, 2020

We likely need to update the build server anyways to fix https://bugs.launchpad.net/mixxx/+bug/1871238

@daschuer
Copy link
Copy Markdown
Member

daschuer commented Apr 9, 2020

OK, so lets go step by step and hold this back until the server was updated.

@daschuer
Copy link
Copy Markdown
Member

daschuer commented Apr 9, 2020

This should be work as intermediated fix: #2641

@uklotzde
Copy link
Copy Markdown
Contributor Author

uklotzde commented Apr 9, 2020

I don't think that we need to upgrade the server. The server runs macOS 10.13 and the corresponding XCode version should be compatible.

@uklotzde
Copy link
Copy Markdown
Contributor Author

uklotzde commented Apr 9, 2020

@daschuer Why introduce another hack instead of trying to remove an obsolete one. The macOS failure might be caused by the hack that I once introduced for Xenial.

@uklotzde
Copy link
Copy Markdown
Contributor Author

uklotzde commented Apr 9, 2020

I'm not able able to judge if the current code in util/optional.h is correct. It worked until now on all platforms, that's all what I can say about it. Therefore I propose to remove it.

@Be-ing
Copy link
Copy Markdown
Contributor

Be-ing commented Apr 9, 2020

According to https://en.wikipedia.org/wiki/Xcode#Xcode_11.x_(since_SwiftUI_framework) XCode 11.3 which we probably need for the crash bug requires at least macOS 10.14.4

@uklotzde
Copy link
Copy Markdown
Contributor Author

uklotzde commented Apr 9, 2020

Changing the build environment should only be the final step. Upgrading would exclude many macOS users with old hardware. But it is also unacceptable to tolerate Apple's business policy, by letting us do all the work to support their customers they decided to leave behind.

...to match our build server that is running macOS 10.13
@uklotzde uklotzde changed the title Remove <experimental/optional> hack from util/optional.h [WiP] Remove <experimental/optional> hack from util/optional.h Apr 9, 2020
@uklotzde
Copy link
Copy Markdown
Contributor Author

uklotzde commented Apr 9, 2020

Experimental: I have downgraded XCode 11.0 to XCode 9.3 on Travis to match our build server environment.

@uklotzde
Copy link
Copy Markdown
Contributor Author

uklotzde commented Apr 9, 2020

Unfortunately doesn't seem to be available in Xcode 9.3 (and the cmake version is also too old):

src/util/optional.h:3:10: fatal error: 'optional' file not found
#include <optional>
         ^~~~~~~~~~

@uklotzde uklotzde closed this Apr 9, 2020
@uklotzde uklotzde deleted the util_optional branch April 9, 2020 21:41
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.

3 participants