Skip to content

Add EasyBlock for cryptography to fix missing -pthread for all versions#1874

Merged
boegel merged 2 commits intoeasybuilders:developfrom
Flamefire:crypto
Dec 26, 2019
Merged

Add EasyBlock for cryptography to fix missing -pthread for all versions#1874
boegel merged 2 commits intoeasybuilders:developfrom
Flamefire:crypto

Conversation

@Flamefire
Copy link
Copy Markdown
Contributor

This fixes easybuilders/easybuild-easyconfigs#9446 for all versions of cryptography as all suffer from the same issue. See pyca/cryptography#5084

The approach was taken from https://github.com/bear-rsg/easybuild-easyconfigs/blob/2019b/easybuild/easyconfigs/p/Python/Python-3.7.4-GCCcore-8.3.0.eb#L119

The alternative would be to go and fix every single EC installing the package to add this to the preinstallopts.

Note also that I'm adding -pthread as opposed to -lpthread because the former a) is independent of the position in the command line and b) may add other compile definitions as required. Cursory check seems to verify that all used compilers support this flag: clang, gcc, icc, pgcc (at least via patch)

Comment thread easybuild/easyblocks/c/cryptography.py
Comment thread easybuild/easyblocks/c/cryptography.py
Comment thread easybuild/easyblocks/c/cryptography.py Outdated
@Flamefire
Copy link
Copy Markdown
Contributor Author

@boegel I created an upstream PR which is likely to be accepted and fix the issue: pyca/cryptography#5086

As this is a blocker for TensorFlow on Power I'd like a decision here:

  1. Use this easyblock with version guard due to the huge amount of versions affected
  2. Extract a patch for Python 3.7.4 (and any EC using Paramiko >= 2.5.0) to allow the sanity check to pass and backporting to older ECs when someone actually uses that part of cryptography on an affected system
  3. Add the patch to all ECs using cryptography

@boegel
Copy link
Copy Markdown
Member

boegel commented Dec 10, 2019

I don't mind adding a custom easyblock for cryptography to resolve this problem in one single place at all, that's exactly what easyblocks are for 👍

If your patch is indeed fixed upstream we can make this fix conditional so it's only done for older versions.

Why hasn't this popped up for others yet though, that surprises me a bit... The problem doesn't look specific to POWER at all?

Copy link
Copy Markdown
Member

@boegel boegel left a comment

Choose a reason for hiding this comment

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

@Flamefire Does it also make sense to run python -c "from cryptography.hazmat.bindings._openssl import lib" as a custom sanity check command, since we know that triggers the problem being fixed here?

Comment thread easybuild/easyblocks/c/cryptography.py Outdated
@Flamefire
Copy link
Copy Markdown
Contributor Author

Why hasn't this popped up for others yet though, that surprises me a bit... The problem doesn't look specific to POWER at all?

See my comment easybuilders/easybuild-easyconfigs#9446 (comment). The TL&DR is: On (e.g.) Power libpthread is a linker script which links in a static part of pthread in addition. It needs to come after the library using it.

Additionally this specific extension doesn't seem to be used that often and an import check did not find it until a change in another module exposed it.

If your patch is indeed fixed upstream we can make this fix conditional so it's only done for older versions.

I'm waiting for it to be merged. Note that my patch is slightly different there: Here I use -pthread, there I append -lpthread. The former is supposedly independent of the position but maybe not universally supported. But without a patch I don't see how to achieve appending the option in an EB.

Comment thread easybuild/easyblocks/c/cryptography.py
Copy link
Copy Markdown
Contributor Author

@Flamefire Flamefire left a comment

Choose a reason for hiding this comment

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

Does it also make sense to run python -c "from cryptography.hazmat.bindings._openssl import lib" as a custom sanity check command, since we know that triggers the problem being fixed here?

Done, good idea 👍

@boegel
Copy link
Copy Markdown
Member

boegel commented Dec 13, 2019

@Flamefire Should we put this on hold until the upstream fix is merged so we can add the version condition?

@Flamefire
Copy link
Copy Markdown
Contributor Author

I wouldn't wait (to long). This does not conflict with a potentially fixed upstream so a later added version check does not hurt. Up to you.

@boegel
Copy link
Copy Markdown
Member

boegel commented Dec 13, 2019

Is this sufficient to make the vanilla Python-3.7.4-GCCcore-8.3.0.eb pass on POWER?

If so, can you upload a test report and share it here?

@Flamefire
Copy link
Copy Markdown
Contributor Author

Yes. Will do on monday

@Flamefire
Copy link
Copy Markdown
Contributor Author

@Flamefire
Copy link
Copy Markdown
Contributor Author

pyca/cryptography#5086 has been merged, so the next release will hopefully contain this fix

@boegel
Copy link
Copy Markdown
Member

boegel commented Dec 26, 2019

Tested this with several Python old & recent easyconfigs (on x86-64), no problems encountered, so going in...

@boegel boegel merged commit 191e2f8 into easybuilders:develop Dec 26, 2019
@Flamefire Flamefire deleted the crypto branch December 27, 2019 11:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Crypthography python package broken on Power9 due to undefined symbol: pthread_atfork

3 participants