Revert "atom: fix build on macOS 10.14"

[michaellass]

Description

The workaround introduced for macOS 10.14 is not necessary anymore as atom 1.31.2 builds fine on Mojave out of the box.

Finding the upstream change that fixed this likely is not worth the effort since many nodejs packages are built during installation and we don't even know which one caused the issue in the first place.

This reverts commit 08c7fb6.

Type(s)

• bugfix
• enhancement
• security fix

Tested on

macOS 10.14 18A391
Xcode 10.0 10A255

Verification

Have you

• followed our Commit Message Guidelines?
• squashed and minimized your commits?
• checked that there aren't other open pull requests for the same change?
• referenced existing tickets on Trac with full URL?

• checked your Portfile with port lint?
• tried a full install with sudo port -vst install? (I did only without -t)
• tested basic functionality of all binary files?

[macportsbot]

Notifying maintainers:
@kurthindenburg for port atom.

[pmetzger]

@kurthindenburg ?

[michaellass]

Alternatively we could also apply configure.*flags and macosx_deployment_target unconditionally as suggested by @ryandesign in 08c7fb6#r30826316. However it's not required anymore.

[kurthindenburg]

I don't have Mojave so can't really comment on it.

[pmetzger]

This change seems to have failed the CI system, btw.

[ryandesign]

Merging this PR would cause the build to no longer use:

                         CFLAGS="${configure.cflags}" \
                         CXXFLAGS="${configure.cxxflags}" \
                         LDFLAGS="${configure.ldflags}" 

Don't we still want to use that?

Alternatively we could also apply configure.*flags and macosx_deployment_target unconditionally as suggested by @ryandesign in 08c7fb6#r30826316. However it's not required anymore.

MacPorts already sets the MACOSX_DEPLOYMENT_TARGET correctly (to the OS version, or the value specified in macports.conf). So long as the build system does not override that, it will do the right thing. node was apparently overriding this before, which was what caused the problem. If they fixed the problem by no longer overriding it, then all is well and we no longer need to set -mmacosx-version-min. If on the other hand they are still overriding it but fixed the problem another way, then we still want to set -mmacosx-version-min=${macosx_deployment_target} to override their override.

This change seems to have failed the CI system, btw.

It succeeded on Xcode 10 and Xcode 9. It failed on Xcode 8 because coreutils could not be extracted because xz was not in the path; see ticket 57457. It failed on Xcode 7 because the github.com SSL certificate allegedly could not be verified.

[pmetzger]

It failed on Xcode 7 because the github.com SSL certificate allegedly could not be verified.

Travis leaves a lot to be desired, and the lack of a really reliable CI system makes it harder to maintain MacPorts. Having our own might be a good thing someday.

[michaellass]

Merging this PR would cause the build to no longer use:

                         CFLAGS="${configure.cflags}" \
                         CXXFLAGS="${configure.cxxflags}" \
                         LDFLAGS="${configure.ldflags}" 

Don't we still want to use that?

That's to decide. I added it at that time since it felt bad to override CFLAGS without applying the macports defaults.

MacPorts already sets the MACOSX_DEPLOYMENT_TARGET correctly (to the OS version, or the value specified in macports.conf). So long as the build system does not override that, it will do the right thing. node was apparently overriding this before, which was what caused the problem. If they fixed the problem by no longer overriding it, then all is well and we no longer need to set -mmacosx-version-min. If on the other hand they are still overriding it but fixed the problem another way, then we still want to set -mmacosx-version-min=${macosx_deployment_target} to override their override.

To be honest: I don't know what node does. It seems to me like nearly every node module itself specifies the build flags again (i.e., target version and/or c++ standard library to use). My guess is that one of atom's dependencies now raised the version or added the flag to use libc++ and atom in version 1.32 updated to that.

Originally I thought node itself applies a default (as suggested by https://github.com/nodejs/node/blob/master/common.gypi#L464 and nodejs/node-gyp#469) but I couldn't find that common.gypi in our installation. If it does, we should probably fix at right there.

[pmetzger]

This PR is getting rather old. What do we need to do in order to resolve it?

[michaellass]

For me it would be fine to just close it. The additional code in the portfile does not hurt anybody. The main reason for this PR was just to simplify the portfile since the workaround is not required anymore.
If there isn't one yet, I'm planning to create a PR to update atom to the most recent version. I could implement the suggestions by @ryandesign then as well.

[pmetzger]

Okay, please do submit a PR to update at some point soon then, and if you want to re-open this PR we can also do that.