Skip to content

deps: add s390 asm rules for OpenSSL-1.1.1#8351

Closed
sam-github wants to merge 1 commit into
openssl:OpenSSL_1_1_1-stablefrom
sam-github:s390-asm-rules-1.1.1
Closed

deps: add s390 asm rules for OpenSSL-1.1.1#8351
sam-github wants to merge 1 commit into
openssl:OpenSSL_1_1_1-stablefrom
sam-github:s390-asm-rules-1.1.1

Conversation

@sam-github

@sam-github sam-github commented Feb 26, 2019

Copy link
Copy Markdown
Contributor

Generate asm files with Makefile rules.

From:

Checklist
  • documentation is added or updated
  • tests are added or updated

@openssl-machine openssl-machine added the hold: cla required The contributor needs to submit a license agreement label Feb 26, 2019
@sam-github

sam-github commented Feb 26, 2019

Copy link
Copy Markdown
Contributor Author

Hi, we are floating this patch in Node.js, are you interested in accepting it upstream?

Most of the changes in it are already on master, but came in with large cleanups, I think that's why they didn't get backported to 1.1.1.

The only line not on master is:

diff --git a/crypto/poly1305/build.info b/crypto/poly1305/build.info
index de44bb8803..9cf65284de 100644
--- a/crypto/poly1305/build.info
+++ b/crypto/poly1305/build.info
@@ -19,3 +19,4 @@ GENERATE[poly1305-mips.S]=asm/poly1305-mips.pl $(PERLASM_SCHEME)
 INCLUDE[poly1305-mips.o]=..
 GENERATE[poly1305-c64xplus.S]=asm/poly1305-c64xplus.pl $(PERLASM_SCHEME)
 GENERATE[poly1305-s390x.S]=asm/poly1305-s390x.pl $(PERLASM_SCHEME)
+INCLUDE[poly1305-s390x.o]=..

I can PR that to master if its wanted.

@sam-github

sam-github commented Feb 26, 2019

Copy link
Copy Markdown
Contributor Author

@shigeki to upstream this, if acceptable, it might need a CLA from you, unless OpenSSL is comfortable calling it trivial, since its a couple lines of configuration, not code.

@sam-github

sam-github commented Feb 26, 2019

Copy link
Copy Markdown
Contributor Author

@levitte should I open a PR to master for the one-liner I show in the diff above?

Full disclosure: I'm just shovelling code between repos, I've no idea how it works, so maybe the one-line isn't necessary.

t-j-h
t-j-h previously approved these changes Feb 26, 2019
View reviewed changes
@levitte

levitte commented Feb 27, 2019

Copy link
Copy Markdown
Member

I have access to a s390, so I can check.

levitte
levitte requested changes Feb 27, 2019
View reviewed changes
Comment thread crypto/poly1305/build.info Outdated
@levitte

levitte commented Feb 27, 2019

Copy link
Copy Markdown
Member

The INCLUDE suggested for master isn't necessary, which had me looking at 1.1.1 as well.

@levitte

levitte commented Feb 27, 2019

Copy link
Copy Markdown
Member

So about that CLA thing... I'm unsure if this can be called trivla from a copyright perspective, so I do believe we need a signed CLA from @shigeki .

@sam-github

sam-github commented Feb 27, 2019

Copy link
Copy Markdown
Contributor Author

4 lines of cut-n-paste config seem trivial, but it might be easier to get @shigeki to do a CLA, if he can.

@shigeki I'd like to upstream this so it doesn't need to be a floating node.js patch, would you be able to sign the CLA?

@shigeki @sam-github
deps: add s390 asm rules for OpenSSL-1.1.1
695b2d9 
Generate asm files with Makefile rules.

From:
- nodejs/node@0d9a86c
@levitte

levitte commented Feb 27, 2019

Copy link
Copy Markdown
Member

("trivial" from a copyright perspective and from a technical perspective is hugely different, FYI. I do agree that this is trivial from a technical perspective, but that's not what counts with regards to CLAs)

@shigeki

shigeki commented Feb 28, 2019

Copy link
Copy Markdown
Contributor

@sam-github Thanks for submitting this on behalf of me.

@levitte I sent my ICLA. Please make sure that you received it.

@mattcaswell

mattcaswell commented Feb 28, 2019

Copy link
Copy Markdown
Member

CLA received. Close/reopen to kick CLA bot.

@mattcaswell mattcaswell reopened this Feb 28, 2019
@openssl-machine openssl-machine removed the hold: cla required The contributor needs to submit a license agreement label Feb 28, 2019
levitte
levitte approved these changes Feb 28, 2019
View reviewed changes

@levitte levitte left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Reconfirming

@levitte levitte added approval: review pending This pull request needs review by a committer branch: 1.1.1 Applies to OpenSSL_1_1_1-stable branch (EOL) labels Feb 28, 2019
@levitte levitte dismissed t-j-h’s stale review February 28, 2019 12:03

Needs reconfirmation

@sam-github

sam-github commented Feb 28, 2019

Copy link
Copy Markdown
Contributor Author

Checks all passed, including CLA. Thanks @shigeki

@bernd-edlinger bernd-edlinger added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Feb 28, 2019
levitte pushed a commit that referenced this pull request Mar 1, 2019
@shigeki @levitte
deps: add s390 asm rules for OpenSSL-1.1.1
de4fb43 
Generate asm files with Makefile rules.

From:
- nodejs/node@0d9a86c

Reviewed-by: Bernd Edlinger <bernd.edlinger@hotmail.de>
Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from #8351)
@levitte

levitte commented Mar 1, 2019

Copy link
Copy Markdown
Member

Merged.

de4fb43 deps: add s390 asm rules for OpenSSL-1.1.1

@levitte levitte closed this Mar 1, 2019
@sam-github sam-github deleted the s390-asm-rules-1.1.1 branch March 1, 2019 19:54
@sam-github

sam-github commented Mar 1, 2019

Copy link
Copy Markdown
Contributor Author

Thank you!

@sam-github sam-github mentioned this pull request Mar 1, 2019
Closed
4 tasks
sam-github added a commit to sam-github/node that referenced this pull request Jun 13, 2019
@sam-github
deps: updated openssl upgrade instructions
a15b6fa 
Patching the s390 asm rules is no longer required.

See: openssl/openssl#8351
sam-github added a commit to sam-github/node that referenced this pull request Jun 13, 2019
@sam-github
deps: updated openssl upgrade instructions
ae2a109 
Patching the s390 asm rules is no longer required.

See: openssl/openssl#8351
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Jun 17, 2019
@sam-github @BridgeAR
deps: updated openssl upgrade instructions
5990c4d 
Patching the s390 asm rules is no longer required.

See: openssl/openssl#8351

PR-URL: nodejs#28211
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
BridgeAR pushed a commit to nodejs/node that referenced this pull request Jun 17, 2019
@sam-github @BridgeAR
deps: updated openssl upgrade instructions
e46d9e9 
Patching the s390 asm rules is no longer required.

See: openssl/openssl#8351

PR-URL: #28211
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
targos pushed a commit to nodejs/node that referenced this pull request Jun 18, 2019
@sam-github @targos
deps: updated openssl upgrade instructions
9419daf 
Patching the s390 asm rules is no longer required.

See: openssl/openssl#8351

PR-URL: #28211
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
BethGriggs pushed a commit to nodejs/node that referenced this pull request Jul 25, 2019
@sam-github @BethGriggs
deps: updated openssl upgrade instructions
c59e0c2 
Patching the s390 asm rules is no longer required.

See: openssl/openssl#8351

PR-URL: #28212
Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@levitte levitte levitte approved these changes
@bernd-edlinger bernd-edlinger bernd-edlinger approved these changes
@t-j-h t-j-h t-j-h left review comments

Assignees

No one assigned

Labels

approval: done This pull request has the required number of approvals branch: 1.1.1 Applies to OpenSSL_1_1_1-stable branch (EOL)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@sam-github @levitte @shigeki @mattcaswell @t-j-h @bernd-edlinger @openssl-machine