openssl3: Drop patches that may no longer be needed

[neverpanic]

Description

The build fails when compiling for i386 because the patches don't apply. It seems openssl/openssl#18056 should fix this already, and if it doesn't we should probably use -DBROKEN_CLANG_ATOMICS instead.

Since I cannot test this due to lack of a machine that supports i386, this is a shot in the dark.

Closes: https://trac.macports.org/ticket/65111

Type(s)

• bugfix
• enhancement
• security fix

Tested on

macOS 12.3.1 21E258 arm64
Command Line Tools 13.3.1.0.1.1648687083
(NOTE: This configuration doesn't do anything for the problem in question, more tests welcome).

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 --nitpick?
• tried existing tests with sudo port test?
• tried a full install with sudo port -vst install?
• tested basic functionality of all binary files?

[macportsbot]

Notifying maintainers:
@larryv for port openssl3.

[Gcenx]

I’ve not tested that option yet so let me give it a go, if that fails there’s already an updated version of this patch

[neverpanic]

I saw that, but I'd rather not continue to apply a patch that isn't getting upstream review and thus will never have a chance to be integrated if there is a better alternative. This patch will keep breaking downstream, and I will forget to update it again because it's not in the set of configurations I test before committing, so getting rid of it if possible is the right choice.

[Gcenx]

I’ll be home in front of my Mac shortly so I can verify if i386 works as is or if the other option is needed.

[Gcenx]

The only way I’ve gotten this to compile as +universal (i386 & x86_64) is ether applying the updated patch or forcing -DBROKEN_CLANG_ATOMICS for i386/universal build that contains i386.

[Gcenx]

Using the following worked for me

# https://github.com/openssl/openssl/issues/16551
# Fixes "Undefined symbols for architecture i386: ___atomic_is_lock_free"
if {(${configure.build_arch} eq "i386") || (${universal_possible} && [variant_isset universal] && "i386" in ${configure.universal_archs})} {
    configure.args-append   -DBROKEN_CLANG_ATOMICS
}

[kencu]

all this sadness stemming from @catap’s command line tools being out of date…

sigh….

[ryandesign]

Removing the patch as in this PR worked to build non-universal on Tiger i386.

Tested on

Mac OS X 10.4.11 8S2167 i386
Xcode 2.5

[Gcenx]

Removing the patch as in this PR worked to build non-universal on Tiger i386.

Tested on

Mac OS X 10.4.11 8S2167 i386 Xcode 2.5

Is this being compiled using Xcode (llvm-gcc) or a macports-clang compiler?, as the problem is with clang handling of atomics for i386.

[catap]

all this sadness stemming from @catap’s command line tools being out of date…

sigh….

from my point of view this is a good outcome: instead of some tricky patch a simple -DBROKEN_CLANG_ATOMICS :)

[kencu]

no atomics = needlessly slower…

but the proper fix, where this patch came from, seems stalled upstream

openssl/openssl#16584

so MacPorts can go back to using it when committed I guess, if it ever is…

[Gcenx]

no atomics = needlessly slower…

but the proper fix, where this patch came from, seems stalled upstream

openssl/openssl#16584

so MacPorts can go back to using it when committed I guess, if it ever is…

Welp it’s ether needlessly slower or outright broken for affected systems without the updated patch.

[kencu]

yes, outdated patches don’t tend to apply very well.

[Gcenx]

yes, outdated patches don’t tend to apply very well.

We have an updated patch already from @catap but won’t be accepted into the Port

[neverpanic]

Using the following worked for me

# https://github.com/openssl/openssl/issues/16551
# Fixes "Undefined symbols for architecture i386: ___atomic_is_lock_free"
if {(${configure.build_arch} eq "i386") || (${universal_possible} && [variant_isset universal] && "i386" in ${configure.universal_archs})} {
    configure.args-append   -DBROKEN_CLANG_ATOMICS
}

I pushed a new version with this change, thanks for testing.

We have an updated patch already from @catap but won’t be accepted into the Port

The correct way to get this fixed is to get the patch applied upstream. I don't want to maintain this patch downstream, and since the upstream integration seems stalled shipping the patch in MacPorts would mean maintaining it downstream.

This will also lead to a bad user experience, because I'm likely to forget the patch in the next update, while the upstream option will probably continue working. In my opinion, it is the better option for this reason.

[Gcenx]

If @catap is willing to send it upstream I’ll make myself available for testing i386.

While using this setting isn’t ideal it’s better having something working slowly than not working at all.

[neverpanic]

Does somebody on i386 want to test that this Portfile works, or should I just merge it to fix the failure in the spirit of "it can't be worse than it is right now?"

[Gcenx]

Does somebody on i386 want to test that this Portfile works, or should I just merge it to fix the failure in the spirit of "it can't be worse than it is right now?"

As you added the section I tested with I'd say just merge it

[barracuda156]

By the way, openssl3 @3.0.3 failed to build for me on 10.6 PPC when I used gcc11, complaining about atomics. Using the old default gcc-4.2 it has built though.

[catap]

If @catap is willing to send it upstream I’ll make myself available for testing i386.

I’m not so comfortable to send a patch which I can’t test :)

i may rebase it as blind shot like I did, but sending to upstream… well.. let me think about

[barracuda156]

If @catap is willing to send it upstream I’ll make myself available for testing i386.

I’m not so comfortable to send a pitch which I can’t test :)

i may rebase it as blind shot like I did, but sending to upstream… well.. let me think about

@catap Could you take a look at the error log from 10.6 PPC?

[kencu]

Well I believe the patch is already upstream, where it first came from, here openssl/openssl#16584 so I don't think anybody needs to send anything upstream.

If upstream commits it, MacPorts can turn atomics back on again if they want I guess.

For now, the mainstream of MacPorters will just do without atomics. Probably nobody will ever be able to tell the difference anyway.

[Gcenx]

@kencu yes but that draft won’t be updated by the author so it won’t ever be merged at this rate. If you remember I’m the one who had that patch pulled in.

[kencu]

I remember.

What is the shame here is that the patch is right. 10 seconds to update it would have had us back where we were (where I am). Instead, for no good reason, we go down this rabbit hole of disabling atomics for a 100% wrong reason (outdated CLTs).

Now that we have this atomic-disabling solution, there will be less impetus for the proper fix to be committed, and we wind up with a needlessly inferior situation.

Kind of a classic open-source issue, and it's really too bad -- I tried to shut this down early to prevent this by helping @catap get his system in order, but the cat was already out of the bag...

Which is why I have my own fork of so many ports.

[neverpanic]

What is the shame here is that the patch is right. 10 seconds to update it would have had us back where we were (where I am). Instead, for no good reason, we go down this rabbit hole of disabling atomics for a 100% wrong reason (outdated CLTs).

Except it wasn't 10 seconds. I spent about 10 minutes starting to fix the patch before deciding I don't want to keep doing that with every new OpenSSL version. This needs to go upstream.