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

crypto/sha/asm/sha512-ia64.pl: When checking assembler file names, ignore case #19604

Closed
wants to merge 1 commit into from

Conversation

levitte
Copy link
Member

@levitte levitte commented Nov 4, 2022

The use case is that uppercase .ASM extension may be used on some platforms,
and we were only testing for the lowercase extension.

…nore case

The use case is that uppercase .ASM extension may be used on some platforms,
and we were only testing for the lowercase extension.
@levitte levitte added approval: otc review pending This pull request needs review by an OTC member approval: review pending This pull request needs review by a committer branch: 3.0 Merge to openssl-3.0 branch branch: 3.1 Merge to openssl-3.1 branch: master Merge to master branch severity: urgent Fixes an urgent issue (exempt from 24h grace period) labels Nov 4, 2022
@levitte levitte self-assigned this Nov 4, 2022
@levitte levitte requested review from a team November 4, 2022 09:55
Copy link
Member

@beldmit beldmit left a comment

Choose a reason for hiding this comment

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

LGTM, approvd if tests passed, agree urgent

@beldmit beldmit removed the approval: otc review pending This pull request needs review by an OTC member label Nov 4, 2022
@levitte
Copy link
Member Author

levitte commented Nov 4, 2022

... approvd if tests passed ...

Yeah well, GitHub Actions won't verify that, and VMS builds are only done on push (because much too slow for PRs), so you'll have to take my word for it that my manual builds compile that assembler file fine with this change.

@mattcaswell
Copy link
Member

Is this causing a CI failure somewhere? I assume with VMS. If so agree urgent.

@mattcaswell mattcaswell added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: review pending This pull request needs review by a committer labels Nov 4, 2022
@levitte
Copy link
Member Author

levitte commented Nov 4, 2022

Is this causing a CI failure somewhere? I assume with VMS. If so agree urgent.

It hasn't showed up in our builds yet... I had a manual build ongoing that I only updated with #19598 and remade, and failure to run this perlasm script came withing minutes. Because our VMS workers are commanded to rebuild from a clean directory, it will take a while before those builds get to the same spot.

Copy link
Member

@t8m t8m left a comment

Choose a reason for hiding this comment

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

OK with urgent

@levitte
Copy link
Member Author

levitte commented Nov 4, 2022

Merged

master:
119b7b5 crypto/sha/asm/sha512-ia64.pl: When checking assembler file names, ignore case

3.1:
733084a crypto/sha/asm/sha512-ia64.pl: When checking assembler file names, ignore case

3.0:
22530d3 crypto/sha/asm/sha512-ia64.pl: When checking assembler file names, ignore case

@levitte levitte closed this Nov 4, 2022
openssl-machine pushed a commit that referenced this pull request Nov 4, 2022
…nore case

The use case is that uppercase .ASM extension may be used on some platforms,
and we were only testing for the lowercase extension.

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
(Merged from #19604)

(cherry picked from commit 119b7b5)
openssl-machine pushed a commit that referenced this pull request Nov 4, 2022
…nore case

The use case is that uppercase .ASM extension may be used on some platforms,
and we were only testing for the lowercase extension.

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
(Merged from #19604)
openssl-machine pushed a commit that referenced this pull request Nov 4, 2022
…nore case

The use case is that uppercase .ASM extension may be used on some platforms,
and we were only testing for the lowercase extension.

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
(Merged from #19604)

(cherry picked from commit 119b7b5)
@levitte levitte deleted the fix-sha512-ia64-20221104 branch November 4, 2022 10:39
@levitte
Copy link
Member Author

levitte commented Nov 4, 2022

And hopefully, there aren't more nits like this. Buildbot will tell...

beldmit pushed a commit to beldmit/openssl that referenced this pull request Dec 26, 2022
…nore case

The use case is that uppercase .ASM extension may be used on some platforms,
and we were only testing for the lowercase extension.

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
(Merged from openssl#19604)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Merge to master branch branch: 3.0 Merge to openssl-3.0 branch branch: 3.1 Merge to openssl-3.1 severity: urgent Fixes an urgent issue (exempt from 24h grace period)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants