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

build: add mips64el support #27992

Conversation

mutao-loongson
Copy link
Contributor

@mutao-loongson mutao-loongson commented May 31, 2019

build: add support for mips64el

V8 now resume supporting for mipsel/mips64el, the support for mips platform
need to recover.

First, revert commit 9334 e45aa0ed815c2745bcc2bb606d91be64998d.

Second, add linux64-mips64 platform dependent files and update the corresponding
gypi files and header files for OpenSSL compiling.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

V8 now resume supporting for mipsel/mips64el.
This commit add linux64-mips64 platform dependent
files in 'deps/openssl/config/archs/linux64-mips64',
and update the corresponding gypi files and header
files.

Refs: https://groups.google.com/forum/#!topic/v8-dev/oXkv5OVCXyc
@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. openssl Issues and PRs related to the OpenSSL dependency. labels May 31, 2019
@refack
Copy link
Contributor

refack commented May 31, 2019

Hello @mutao-loongson, and thank you for the contribution!

I would really be happy if we could get some sort of CI for this, preferably under the https://github.com/nodejs/unofficial-builds initiative. Could you help with that?

@mutao-loongson
Copy link
Contributor Author

Hello @mutao-loongson, and thank you for the contribution!

I would really be happy if we could get some sort of CI for this, preferably under the https://github.com/nodejs/unofficial-builds initiative. Could you help with that?

I would love to do something for nodejs community. I have read https://github.com/nodejs/unofficial-builds. It only copes with compiling stage of CI, no tests stage.

What you mean by "some sort of CI"? Could you explain further for that ? What I need to do to offer help? And what need to do to make mips64el to be one of the platforms that nodejs officially supports ?

Please give me some instructions.

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

LGTM (and RSLGTM w.r.t. to the openssl changes; they look correct but I didn't check every line.)

Having mipsel machines in the CI matrix would be nice but it's not as if we had those before. I'm okay with landing this as-is as long as it's understood that it comes with no warranty, no promise of support, etc.

(That includes timely review of mipsel-related pull requests ^^)

@mutao-loongson
Copy link
Contributor Author

OK, I willing to spend time to support node for mipsel-related problems. First, I would following the advice from @refack to response to https://github.com/nodejs/unofficial-builds initiative. When opportunity matures, then, I would propose adding mipsel machines into the CI matrix.

Best Regards.

@mutao-loongson
Copy link
Contributor Author

Hello, when could this pull request be landed ?

@nodejs-github-bot
Copy link
Collaborator

@Trott
Copy link
Member

Trott commented Jun 16, 2019

Hello, when could this pull request be landed ?

I believe the only remaining requirement is that it pass CI.

CI: https://ci.nodejs.org/job/node-test-pull-request/23913/

Also: /ping @nodejs/build in case anyone there has opinions or wants to do something CI-wise....

@Trott
Copy link
Member

Trott commented Jun 16, 2019

Also: /ping @nodejs/crypto in case anyone wants to look over the OpenSSL stuff

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 16, 2019
@rvagg rvagg added the mips Issues and PRs related to the MIPS architecture. label Jun 17, 2019
@rvagg
Copy link
Member

rvagg commented Jun 17, 2019

This is really sweet. I don't have full confidence in the OpenSSL changes and would love for @sam-github or @shigeki to add their +1 to @bnoordhuis'.
Long-standing policy for platforms not officially supported is something like - we'll accept changes as long as they don't interfere with supported platforms and are not overly intrusive. These changes are very minimal and certainly pass that test IMO. Great work!

@nodejs-github-bot
Copy link
Collaborator

Copy link
Contributor

@sam-github sam-github left a comment

Choose a reason for hiding this comment

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

As far as I can tell, this is purely additive, it doesn't change anything about node/openssl on currently supported platforms, and is pretty minimal (apart from the noise of auto-generated openssl config output), so LGTM.

@mutao-loongson
Copy link
Contributor Author

Yes, I don't change anything about node/openssl on currently supported platform. I just modified some gypi and header files and added auto-generated openssl config output to add mips64el compiling support. But CI can't pass ?! I am confused! I will setup win2008r2-vs2017 envrionment in virtual machine to try to reproduce the failure and try to figure out what wrong with "node-test-binary-windows-2" test case.

@nodejs-github-bot
Copy link
Collaborator

@rvagg
Copy link
Member

rvagg commented Jun 18, 2019

I think you just hit a flaky test @mutao-loongson, the re-run is green(ish) so this is good to go I think.

@rvagg
Copy link
Member

rvagg commented Jun 18, 2019

Landed in 2a9f1ad...779a243

@rvagg rvagg closed this Jun 18, 2019
rvagg pushed a commit that referenced this pull request Jun 18, 2019
This reverts commit 9334e45.

PR-URL: #27992
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
rvagg pushed a commit that referenced this pull request Jun 18, 2019
V8 now resume supporting for mipsel/mips64el.
This commit add linux64-mips64 platform dependent
files in 'deps/openssl/config/archs/linux64-mips64',
and update the corresponding gypi files and header
files.

Refs: https://groups.google.com/forum/#!topic/v8-dev/oXkv5OVCXyc

PR-URL: #27992
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
@mutao-loongson
Copy link
Contributor Author

Thanks @rvagg , I would not try to reproduce the failure anymore.

@mutao-loongson mutao-loongson deleted the deps-openssl-mips64el-asm-support branch June 19, 2019 02:16
@mutao-loongson mutao-loongson restored the deps-openssl-mips64el-asm-support branch June 19, 2019 02:17
targos pushed a commit that referenced this pull request Jul 2, 2019
This reverts commit 9334e45.

PR-URL: #27992
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
targos pushed a commit that referenced this pull request Jul 2, 2019
V8 now resume supporting for mipsel/mips64el.
This commit add linux64-mips64 platform dependent
files in 'deps/openssl/config/archs/linux64-mips64',
and update the corresponding gypi files and header
files.

Refs: https://groups.google.com/forum/#!topic/v8-dev/oXkv5OVCXyc

PR-URL: #27992
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
targos added a commit that referenced this pull request Jul 2, 2019
Notable changes:

* build:
  * Experimental support for building Node.js on MIPS architecture
    is back. #27992
* child_process:
  * The promisified versions of `child_process.exec` and
    `child_process.execFile` now both return a `Promise` which has the
	child instance attached to their `child` property.
	#28325
* deps:
  * Updated libuv to 1.30.0. #28449
    * Support for the Haiku platform has been added.
    * The maximum `UV_THREADPOOL_SIZE` has been increased from 128 to
	  1024.
    * `uv_fs_copyfile()` now works properly when the source and
	  destination files are the same.
* process:
  * A new method, `process.resourceUsage()` was added. It returns
    resource usage for the current process, such as CPU time.
	#28018
* src:
  * Fixed an issue related to stdio that could lead to a crash of the
    process in some circumstances.
	#28490
* stream:
  * Added a `writableFinished` property to writable streams. It
    indicates that all the data has been flushed to the underlying
	system. #28007
* worker:
  * Fixed an issue that prevented worker threads to listen for data on
    stdin. #28153

PR-URL: #28508
@targos targos mentioned this pull request Jul 2, 2019
targos added a commit that referenced this pull request Jul 2, 2019
Notable changes:

* build:
  * Experimental support for building Node.js on MIPS architecture
    is back. #27992
* child_process:
  * The promisified versions of `child_process.exec` and
    `child_process.execFile` now both return a `Promise` which has the
	child instance attached to their `child` property.
	#28325
* deps:
  * Updated libuv to 1.30.0. #28449
    * Support for the Haiku platform has been added.
    * The maximum `UV_THREADPOOL_SIZE` has been increased from 128 to
	  1024.
    * `uv_fs_copyfile()` now works properly when the source and
	  destination files are the same.
* process:
  * A new method, `process.resourceUsage()` was added. It returns
    resource usage for the current process, such as CPU time.
	#28018
* src:
  * Fixed an issue related to stdio that could lead to a crash of the
    process in some circumstances.
	#28490
* stream:
  * Added a `writableFinished` property to writable streams. It
    indicates that all the data has been flushed to the underlying
	system. #28007
* worker:
  * Fixed an issue that prevented worker threads to listen for data on
    stdin. #28153
* meta:
  * Added Jiawen Geng (https://github.com/gengjiawen) to collaborators.
    #28322

PR-URL: #28508
targos added a commit that referenced this pull request Jul 2, 2019
Notable changes:

* build:
  * Experimental support for building Node.js on MIPS architecture
    is back. #27992
* child_process:
  * The promisified versions of `child_process.exec` and
    `child_process.execFile` now both return a `Promise` which has the
	child instance attached to their `child` property.
	#28325
* deps:
  * Updated libuv to 1.30.1. #28449,
    #28511
    * Support for the Haiku platform has been added.
    * The maximum `UV_THREADPOOL_SIZE` has been increased from 128 to
	  1024.
    * `uv_fs_copyfile()` now works properly when the source and
	  destination files are the same.
* process:
  * A new method, `process.resourceUsage()` was added. It returns
    resource usage for the current process, such as CPU time.
	#28018
* src:
  * Fixed an issue related to stdio that could lead to a crash of the
    process in some circumstances.
	#28490
* stream:
  * Added a `writableFinished` property to writable streams. It
    indicates that all the data has been flushed to the underlying
	system. #28007
* worker:
  * Fixed an issue that prevented worker threads to listen for data on
    stdin. #28153
* meta:
  * Added Jiawen Geng (https://github.com/gengjiawen) to collaborators.
    #28322

PR-URL: #28508
targos added a commit that referenced this pull request Jul 3, 2019
Notable changes:

* build:
  * Experimental support for building Node.js on MIPS architecture
    is back. #27992
* child_process:
  * The promisified versions of `child_process.exec` and
    `child_process.execFile` now both return a `Promise` which has the
	child instance attached to their `child` property.
	#28325
* deps:
  * Updated libuv to 1.30.1. #28449,
    #28511
    * Support for the Haiku platform has been added.
    * The maximum `UV_THREADPOOL_SIZE` has been increased from 128 to
	  1024.
    * `uv_fs_copyfile()` now works properly when the source and
	  destination files are the same.
* process:
  * A new method, `process.resourceUsage()` was added. It returns
    resource usage for the current process, such as CPU time.
	#28018
* src:
  * Fixed an issue related to stdio that could lead to a crash of the
    process in some circumstances.
	#28490
* stream:
  * Added a `writableFinished` property to writable streams. It
    indicates that all the data has been flushed to the underlying
	system. #28007
* worker:
  * Fixed an issue that prevented worker threads to listen for data on
    stdin. #28153
* meta:
  * Added Jiawen Geng (https://github.com/gengjiawen) to collaborators.
    #28322

PR-URL: #28508
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. build Issues and PRs related to build files or the CI. mips Issues and PRs related to the MIPS architecture. openssl Issues and PRs related to the OpenSSL dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants