Skip to content
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.

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

remove disable flag for RTTI in snappy v1.1.9 #15099

Merged

Conversation

lexming
Copy link
Contributor

@lexming lexming commented Mar 9, 2022

(created using eb --new-pr)

Version 1.1.9 of snappy forcefully disables RTTI in GCC, which breaks software still looking for the RTTI symbols. This patch just sets RTTI back to its default.

I hit this issue with dm-reverb v0.7.0 (#15104). However, disabling RTTI will be disruptive in general until all software downstream updates to not rely on typeinfo symbols. Rolling back to have RTTI enabled is very safe as it just adds extra symbols, so software programmed for -fno-rtti should still work, and it is also the default behaviour of GCC.

On a side note, it seems that this change was motivated due to issues with Clang, which is not used in this toolchain. See commit google/snappy@c98344f and its parents.

@lexming
Copy link
Contributor Author

lexming commented Mar 9, 2022

@boegelbot: please test @ generoso

@lexming
Copy link
Contributor Author

lexming commented Mar 9, 2022

Test report by @lexming
SUCCESS
Build succeeded for 1 out of 1 (1 easyconfigs in total)
node302.hydra.os - Linux CentOS Linux 7.9.2009, x86_64, Intel(R) Xeon(R) Gold 6148 CPU @ 2.40GHz, Python 2.7.5
See https://gist.github.com/f5f60971f4afeeadf85589a8ad9558ab for a full test report.

@boegelbot
Copy link
Collaborator

@lexming: Request for testing this PR well received on login1

PR test command 'EB_PR=15099 EB_ARGS= /opt/software/slurm/bin/sbatch --job-name test_PR_15099 --ntasks=4 ~/boegelbot/eb_from_pr_upload_generoso.sh' executed!

  • exit code: 0
  • output:
Submitted batch job 8252

Test results coming soon (I hope)...

- notification for comment with ID 1063017188 processed

Message to humans: this is just bookkeeping information for me,
it is of no use to you (unless you think I have a bug, which I don't).

@boegelbot
Copy link
Collaborator

Test report by @boegelbot
SUCCESS
Build succeeded for 1 out of 1 (1 easyconfigs in total)
cns4 - Linux Rocky Linux 8.5, x86_64, Intel(R) Xeon(R) CPU E5-2667 v3 @ 3.20GHz (haswell), Python 3.6.8
See https://gist.github.com/0f67df9d7b4c60565854aa7d889e7888 for a full test report.

@casparvl
Copy link
Contributor

casparvl commented Mar 9, 2022

Test report by @casparvl
SUCCESS
Build succeeded for 1 out of 1 (1 easyconfigs in total)
software2.lisa.surfsara.nl - Linux debian 10.11, x86_64, Intel(R) Xeon(R) Bronze 3104 CPU @ 1.70GHz, 4 x NVIDIA NVIDIA TITAN V, 470.103.01, Python 2.7.16
See https://gist.github.com/763807fb7c457c77e72544ae7648397b for a full test report.

@casparvl
Copy link
Contributor

casparvl commented Mar 9, 2022

The only thing I was concerned about with this PR is what the reason was that RTTI might have been disabled - if we re-enable it and it was disabled for good reason, we might shoot ourselves in the foot.

I did a deep-dive into the CMakeLists.txt history, it seems disabling was introduced in this commit. From the commit message "Fix CLANG/GCC compilation warnings" I'd say were pretty safe as long as it compiles - and it seems it does.

Copy link
Contributor

@casparvl casparvl left a comment

Choose a reason for hiding this comment

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

Might be good to mention the specific software that hit the RTTI issue. Other than that, looks good to me.

@casparvl casparvl added this to the next release (4.5.4?) milestone Mar 11, 2022
Copy link
Contributor

@casparvl casparvl left a comment

Choose a reason for hiding this comment

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

LGTM!

@casparvl
Copy link
Contributor

Going in, thanks @lexming!

@casparvl casparvl merged commit 053cfef into easybuilders:develop Mar 11, 2022
@lexming lexming deleted the 20220309155425_new_pr_snappy119 branch March 11, 2022 22:17
@lexming
Copy link
Contributor Author

lexming commented Oct 28, 2022

@boegelbot please test @ jsc-zen2

@boegelbot
Copy link
Collaborator

@lexming: Request for testing this PR well received on jsczen2l1.int.jsc-zen2.easybuild-test.cluster

PR test command 'EB_PR=15099 EB_ARGS= /opt/software/slurm/bin/sbatch --job-name test_PR_15099 --ntasks=8 ~/boegelbot/eb_from_pr_upload_jsc-zen2.sh' executed!

  • exit code: 0
  • output:
Submitted batch job 1741

Test results coming soon (I hope)...

- notification for comment with ID 1295042240 processed

Message to humans: this is just bookkeeping information for me,
it is of no use to you (unless you think I have a bug, which I don't).

@boegelbot
Copy link
Collaborator

Test report by @boegelbot
SUCCESS
Build succeeded for 1 out of 1 (1 easyconfigs in total)
jsczen2c1.int.jsc-zen2.easybuild-test.cluster - Linux Rocky Linux 8.5, x86_64, AMD EPYC 7742 64-Core Processor (zen2), Python 3.6.8
See https://gist.github.com/add1ee43c7f844c0e22742735d8b7911 for a full test report.

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.

3 participants