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: cipher.js refactoring #20164

Closed
wants to merge 3 commits into from
Closed

Conversation

danbev
Copy link
Contributor

@danbev danbev commented Apr 20, 2018

This pull request contains three commits that aims to reduce code duplication in lib/internal/crypto/cipher.js
Please refer to the individual commit messages for details about the commits.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

This commit extracts the common code from the Cipher/Cipheriv and
Decipher/Decipheriv constructors into a separate function to avoid
code duplication.
This commit adds a function named addCipherPrototypeFunctions to avoid
code duplication.
This commit renames rsaPublic and removes the rsaPrivate function as the
code in these two functions are identical.
@nodejs-github-bot nodejs-github-bot added the crypto Issues and PRs related to the crypto subsystem. label Apr 20, 2018
@danbev
Copy link
Contributor Author

danbev commented Apr 20, 2018

@danbev
Copy link
Contributor Author

danbev commented Apr 20, 2018

node-test-commit-plinux failure looks unrelated

console output:

make[2]: Leaving directory `/home/iojs/build/workspace/node-test-commit-plinux/nodes/ppcle-ubuntu1404/test/addons/06_factory_of_wrapped_objects/build'
make[2]: write error
gyp ERR! build error 
gyp ERR! stack Error: `make` failed with exit code: 1
gyp ERR! stack     at ChildProcess.onExit (/home/iojs/build/workspace/node-test-commit-plinux/nodes/ppcle-ubuntu1404/deps/npm/node_modules/node-gyp/lib/build.js:258:23)
gyp ERR! stack     at ChildProcess.emit (events.js:182:13)
gyp ERR! stack     at Process.ChildProcess._handle.onexit (internal/child_process.js:222:12)
gyp ERR! System Linux 3.13.0-144-generic
gyp ERR! command "/home/iojs/build/workspace/node-test-commit-plinux/nodes/ppcle-ubuntu1404/out/Release/node" "/home/iojs/build/workspace/node-test-commit-plinux/nodes/ppcle-ubuntu1404/deps/npm/node_modules/node-gyp/bin/node-gyp" "--loglevel=info" "rebuild" "--python=/usr/bin/python" "--directory=/home/iojs/build/workspace/node-test-commit-plinux/nodes/ppcle-ubuntu1404/test/addons/06_factory_of_wrapped_objects/" "--nodedir=/home/iojs/build/workspace/node-test-commit-plinux/nodes/ppcle-ubuntu1404"
gyp ERR! cwd /home/iojs/build/workspace/node-test-commit-plinux/nodes/ppcle-ubuntu1404/test/addons/06_factory_of_wrapped_objects
gyp ERR! node -v v10.0.0-pre
gyp ERR! node-gyp -v v3.6.2
gyp ERR! not ok 
make[1]: *** [test/addons/.buildstamp] Error 1
make[1]: *** Waiting for unfinished jobs....
make[2]: Entering directory `/home/iojs/build/workspace/node-test-commit-plinux/nodes/ppcle-ubuntu1404/test/addons-napi/test_callback_scope/build'
gyp info spawn make

Copy link
Member

@tniessen tniessen left a comment

Choose a reason for hiding this comment

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

Big +1, this has been on my to-do list for ages.

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 22, 2018
@danbev
Copy link
Contributor Author

danbev commented Apr 23, 2018

node-test-commit-plinux failure looks unrelated

console output:

make[2]: Leaving directory `/home/iojs/build/workspace/node-test-commit-plinux/nodes/ppcle-ubuntu1404/test/addons/06_factory_of_wrapped_objects/build'
make[2]: write error
gyp ERR! build error 
gyp ERR! stack Error: `make` failed with exit code: 1
gyp ERR! stack     at ChildProcess.onExit (/home/iojs/build/workspace/node-test-commit-plinux/nodes/ppcle-ubuntu1404/deps/npm/node_modules/node-gyp/lib/build.js:258:23)
gyp ERR! stack     at ChildProcess.emit (events.js:182:13)
gyp ERR! stack     at Process.ChildProcess._handle.onexit (internal/child_process.js:222:12)
gyp ERR! System Linux 3.13.0-144-generic
gyp ERR! command "/home/iojs/build/workspace/node-test-commit-plinux/nodes/ppcle-ubuntu1404/out/Release/node" "/home/iojs/build/workspace/node-test-commit-plinux/nodes/ppcle-ubuntu1404/deps/npm/node_modules/node-gyp/bin/node-gyp" "--loglevel=info" "rebuild" "--python=/usr/bin/python" "--directory=/home/iojs/build/workspace/node-test-commit-plinux/nodes/ppcle-ubuntu1404/test/addons/06_factory_of_wrapped_objects/" "--nodedir=/home/iojs/build/workspace/node-test-commit-plinux/nodes/ppcle-ubuntu1404"
gyp ERR! cwd /home/iojs/build/workspace/node-test-commit-plinux/nodes/ppcle-ubuntu1404/test/addons/06_factory_of_wrapped_objects
gyp ERR! node -v v10.0.0-pre
gyp ERR! node-gyp -v v3.6.2
gyp ERR! not ok 
make[1]: *** [test/addons/.buildstamp] Error 1

@danbev
Copy link
Contributor Author

danbev commented Apr 23, 2018

Landed in 9f97f10, d024c2c, and c851263.

@danbev danbev closed this Apr 23, 2018
danbev added a commit that referenced this pull request Apr 23, 2018
This commit extracts the common code from the Cipher/Cipheriv and
Decipher/Decipheriv constructors into a separate function to avoid
code duplication.

PR-URL: #20164
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
danbev added a commit that referenced this pull request Apr 23, 2018
This commit adds a function named addCipherPrototypeFunctions to avoid
code duplication.

PR-URL: #20164
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
danbev added a commit that referenced this pull request Apr 23, 2018
This commit renames rsaPublic and removes the rsaPrivate function as the
code in these two functions are identical.

PR-URL: #20164
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@jasnell
Copy link
Member

jasnell commented Apr 23, 2018

This does not land cleanly in v10.x-staging as it appears to depend on a semver-major that landed last week that is not going to make it in to 10.0.0. A backport PR may allow this to land in v10.x but it's not going to make it in time for 10.0.0

@MylesBorins
Copy link
Contributor

ping re: backport

tniessen pushed a commit to tniessen/node that referenced this pull request May 13, 2018
This commit extracts the common code from the Cipher/Cipheriv and
Decipher/Decipheriv constructors into a separate function to avoid
code duplication.

PR-URL: nodejs#20164
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
tniessen pushed a commit to tniessen/node that referenced this pull request May 13, 2018
This commit adds a function named addCipherPrototypeFunctions to avoid
code duplication.

PR-URL: nodejs#20164
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
tniessen pushed a commit to tniessen/node that referenced this pull request May 13, 2018
This commit renames rsaPublic and removes the rsaPrivate function as the
code in these two functions are identical.

PR-URL: nodejs#20164
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
addaleax pushed a commit that referenced this pull request May 14, 2018
This commit extracts the common code from the Cipher/Cipheriv and
Decipher/Decipheriv constructors into a separate function to avoid
code duplication.

Backport-PR-URL: #20706
PR-URL: #20164
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
addaleax pushed a commit that referenced this pull request May 14, 2018
This commit adds a function named addCipherPrototypeFunctions to avoid
code duplication.

Backport-PR-URL: #20706
PR-URL: #20164
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
addaleax pushed a commit that referenced this pull request May 14, 2018
This commit renames rsaPublic and removes the rsaPrivate function as the
code in these two functions are identical.

Backport-PR-URL: #20706
PR-URL: #20164
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@addaleax addaleax mentioned this pull request May 14, 2018
@targos targos removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. backport-requested-v10.x labels May 14, 2018
@danbev danbev deleted the cipher_js_refactoring branch May 31, 2018 05:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crypto Issues and PRs related to the crypto subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants