Skip to content

Github Action: brutally try to merge libcrypto and libssl#20308

Closed
levitte wants to merge 3 commits intoopenssl:masterfrom
levitte:fix-20304
Closed

Github Action: brutally try to merge libcrypto and libssl#20308
levitte wants to merge 3 commits intoopenssl:masterfrom
levitte:fix-20304

Conversation

@levitte
Copy link
Member

@levitte levitte commented Feb 16, 2023

This build test does one thing only, it tries to brutally merge libcrypto.a
and libssl.a, thus ensuring that there aren't any duplicate symbols between
them.

Fixes #20304

This build test does one thing only, it tries to brutally merge libcrypto.a
and libssl.a, thus ensuring that there aren't any duplicate symbols between
them.

Fixes openssl#20304
@levitte levitte added branch: master Applies to master branch approval: review pending This pull request needs review by a committer approval: otc review pending labels Feb 16, 2023
@levitte levitte self-assigned this Feb 16, 2023
@levitte levitte requested review from a team February 16, 2023 10:33
Copy link
Contributor

@tmshort tmshort left a comment

Choose a reason for hiding this comment

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

Too bad ar or libtool can't figure this out.

@tmshort tmshort removed the approval: review pending This pull request needs review by a committer label Feb 16, 2023
@mattcaswell mattcaswell added approval: done This pull request has the required number of approvals and removed approval: otc review pending labels Feb 16, 2023
Copy link
Member

@slontis slontis left a comment

Choose a reason for hiding this comment

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

Maybe rename to duplicate_symbol_check.yml ?

@levitte
Copy link
Member Author

levitte commented Feb 17, 2023

Maybe rename to duplicate_symbol_check.yml ?

Not sure why? Do you mean we should have one job per yaml file? I'm rather thinking that this file would collect other special cases...

@slontis
Copy link
Member

slontis commented Feb 17, 2023

Yep - one per job. 'Special' doesn't mean much if I was looking for this particular check.

@levitte
Copy link
Member Author

levitte commented Feb 17, 2023

Yep - one per job. 'Special' doesn't mean much if I was looking for this particular check.

That's note entirely true, but ok, fair enough

@levitte
Copy link
Member Author

levitte commented Feb 17, 2023

Rename done

Copy link
Member

@slontis slontis left a comment

Choose a reason for hiding this comment

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

thanks

@t8m t8m added triaged: feature The issue/pr requests/adds a feature tests: present The PR has suitable tests present labels Feb 17, 2023
@DDvO
Copy link
Contributor

DDvO commented Feb 17, 2023

Why not simply use something like this?

nm -g      libcrypto.a  libssl.a  | awk '{print $3}' | sort | uniq -d
objdump -T libcrypto.so libssl.so | awk '{print $7}' | sort | uniq -d

@tmshort
Copy link
Contributor

tmshort commented Feb 17, 2023

I'm still good with the rename.

@openssl-machine
Copy link
Collaborator

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

@levitte
Copy link
Member Author

levitte commented Feb 18, 2023

Why not simply use something like this?

nm -g      libcrypto.a  libssl.a  | awk '{print $3}' | sort | uniq -d
objdump -T libcrypto.so libssl.so | awk '{print $7}' | sort | uniq -d

Oh, that was actually smart... and actually, we have a test recipe that already plays with nm, so it shouldn't be too hard to extend!

@levitte
Copy link
Member Author

levitte commented Feb 18, 2023

Following @DDvO's question, I submitted an alternative PR, #20311 #20331

@levitte
Copy link
Member Author

levitte commented Feb 24, 2023

Closing, since #20331 is now merged

@levitte levitte closed this Feb 24, 2023
@levitte levitte deleted the fix-20304 branch February 24, 2023 07:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approval: done This pull request has the required number of approvals branch: master Applies to master branch tests: present The PR has suitable tests present triaged: feature The issue/pr requests/adds a feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add a github action to check for duplicate symbols across libssl/libcrypto

7 participants