Make opensslconf.h include depending on target archs #1377

shigeki · 2015-04-08T14:54:31Z

This is a proposal to change deps/openssl/conf/opensslconf.h before upgrading 1.0.2a.

@bnoordhuis @indutny I would like to have your feedback.

The current deps/openssl/conf/opensslconf.h is very hard to maintain with increasing supporting OS and CPU because it is generated by Configure script with specifying a target argument, where it includes sevral defines that depend on OS and architecture platform. See
https://github.com/shigeki/io.js/blob/old_upgrade_openssl102a/deps/openssl/doc/openssl_conf.pdf
is a table at a glance.

A new approach here is that we statically mapped --dest-os and --dest-cpu options in configure to the target of Configure in OpenSSL. The opensslconf.h in each target was created in advance by typing deps/openssl/openssl/Configure {target} and copied into deps/openssl/conf/archs/{target}/opensslconf.h. After that we apply the fix of opensslconf.h according to iojs requirements.

We change deps/openssl/conf/openssconf.h so as to include each file according to its target by checking pre-defined compiler macros.

Here is a map table of configure options in iojs, target arch of Configure in OpenSSL and CI check.

--dest-os	--dest-cpu	OpenSSL target arch	CI
linux	ia32	linux-elf	o
linux	x32	patched linux-x86__64	-
linux	x64	linux-x86__64	o
linux	arm	linux-armv4	o
linux	arm64	N/A	-
mac	ia32	darwin-i386-cc	o
mac	x64	darwin64-x86-cc	o
win	ia32	VC-WIN32	-
win	x64	VC-WIN64A	o
solaris	ia32	solaris-x86-gcc	o
solaris	x64	solaris64-x86__64-gcc	o
freebsd	ia32	BSD-x86	o
freebsd	x64	BSD-x86__64	o
openbsd	ia32	BSD-x86	-
openbsd	x64	BSD-x86__64	-
others	others	linux-elf	-

--dest-os and --dest-cpu are mapped to pre-defined macros.

--dest-os	pre-defined macro
win	__WIN32
win(64bit)	__WIN64
mac	APPLE && MACH
solaris	____sun
freebsd	FreeBSD
openbsd	OpenBSD
linux (not andorid)	linux && !ANDROID
android	ANDROID

--dest-cpu	pre-defined macro
arm	arm
arm64	aarch64
ia32	i386
ia32(win)	__M__IX86
mips	mips
mipsel	MIPSEL
x32	ILP32
x64	x86__64
x64(win)	__M__X64

These are the list which is not implemented yet.

--dest-os	--dest-cpu	OpenSSL target arch	CI
linux	mips	linux-mips32,linux-mips64,linux64-mips64?	---
linux	mipsel	?	---
android	ia32	android-x86	---
android	arm	android-armv7	---
android	mips	android-mips	---
android	mipsel	?	---

Supported target arch list in OpenSSL can be obtained by typing deps/openssl/openssl/Configure LIST.

The CI result is fine with this change as
https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/464/

If this approach is not accepted, I will be back work on it as in shigeki@2d5ca2a that was the same way as we did in the past.

bnoordhuis · 2015-04-08T15:17:19Z

@shigeki Can you add a Makefile target that regenerates the config files? Doesn't have to be the top-level Makefile, adding a new one in deps/openssl is fine too.

indutny · 2015-04-08T15:18:05Z

@bnoordhuis +1

bnoordhuis · 2015-04-08T15:18:42Z

deps/openssl/config/opensslconf.h

To be clear, the uppercase directory name is generated by openssl's Configure script?

Style nit: can you write this as #include or # include? This indent style isn't used anywhere else.

The directory name is the same as the one specified in Configure script. I will change the style to put the hash bangs at the head of line.

shigeki · 2015-04-09T00:29:50Z

@bnoordhuis Adding Makefile is a good idea. I will try it.

shigeki · 2015-04-09T09:47:08Z

I've just updated this to add Makefile that can re-generate files of opensslconf.h for all supported targets and apply fixes to linux-x32, darwin64-x86_64-cc and WIN. But Configure in the iojs repository adds some unnecessary files and overwrites existing files so that I added backup, restore and cleanup rules in Makefile. Styles are also fixed. PTAL.

bnoordhuis · 2015-04-09T09:53:39Z

deps/openssl/config/Makefile

s/ARCS/ARCHS/ maybe?

Right. I will fix this.

indutny · 2015-04-09T09:56:43Z

Yay, looking good to me!

shigeki · 2015-04-09T09:58:11Z

I have to make spell checks for my all commits.

bnoordhuis · 2015-04-09T10:02:17Z

deps/openssl/config/Makefile

Because you're already using perl, would it be an idea to replace the sed one-liners with perl one-liners? That prevents GNU sed vs. BSD sed compatibility issues. You also don't have to escape so much because you can just do perl -i -pe 's!foo/bar!quux/qunc!g'.

Oh, and would it be possible to wrap the long lines?

Okay, I will try it.

shigeki · 2015-04-09T15:40:33Z

@bnoordhuis I've just update Makefile to use perl instead of sed and long lines got shorten to be less than 80. At the previous update, I forgot to include the commit for installer, so added it. All files now were spell-checked.

New CI of https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/474/ seems to be fine except new failures of smartos64 and win2008 but they are not related. It's great arm tests are getting better. Thanks.

bnoordhuis · 2015-04-09T20:29:15Z

There's a small typo in the last commit log (s/archtectures/architectures/) but apart from that LGTM. Nice work.

In OpenSSL, opensslconf.h was generated by Configure script with specifying a target argument, where it includes several defines that depend on OS and architecture platform. In iojs, we statically mapped --dest-os and --dest-cpu options in configure to the target of Configure in OpenSSL and make `deps/openssl/conf/openssconf.h` so as to include each file according to its target by checking pre-defined compiler macros. Included opnesslconf.h files for supported target architectures can be generated by `Makefile` and stored under `archs/{target}/opensslconf.h`. The Makefile alos fixes several defines to meet iojs build requirements. Here is a map table of configure options in iojs, target arch of Configure in OpenSSL and CI support. | --dest-os | --dest-cpu | OpenSSL target arch | CI | | --------- | ---------- | -------------------- | --- | | linux | ia32 | linux-elf | o | | linux | x32 | patched linux-x86_64 | - | | linux | x64 | linux-x86_64 | o | | linux | arm | linux-armv4 | o | | linux | arm64 | N/A | - | | mac | ia32 | darwin-i386-cc | o | | mac | x64 | darwin64-x86-cc | o | | win | ia32 | VC-WIN32 | - | | win | x64 | VC-WIN64A | o | | solaris | ia32 | solaris-x86-gcc | o | | solaris | x64 | solaris64-x86_64-gcc | o | | freebsd | ia32 | BSD-x86 | o | | freebsd | x64 | BSD-x86_64 | o | | openbsd | ia32 | BSD-x86 | - | | openbsd | x64 | BSD-x86_64 | - | | others | others | linux-elf | - | --dest-os and --dest-cpu are mapped to pre-defined macros. | --dest-os | pre-defined macro | | ------------------ | ------------------------- | | win | _WIN32 | | win(64bit) | _WIN64 | | mac | __APPLE__ && __MACH__ | | solaris | __sun | | freebsd | __FreeBSD__ | | openbsd | __OpenBSD__ | | linux (not andorid)| __linux__ && !__ANDROID__ | | android | __ANDROID__ | | --dest-cpu | pre-defined macro | | ---------- | ----------------- | | arm | __arm__ | | arm64 | __aarch64__ | | ia32 | __i386__ | | ia32(win) | _M_IX86 | | mips | __mips__ | | mipsel | __MIPSEL__ | | x32 | __ILP32__ | | x64 | __x86_64__ | | x64(win) | _M_X64 | These are the list which is not implemented yet. | --dest-os | --dest-cpu | OpenSSL target arch | CI | | --------- | ---------- | -------------------- | --- | | linux | mips | linux-mips32,linux-mips64,linux64-mips64? | --- | | linux | mipsel | ? | --- | | android | ia32 | android-x86 | --- | | android | arm | android-armv7 | --- | | android | mips | android-mips | --- | | android | mipsel | ? | --- | Supported target arch list in OpenSSL can be obtained by typing `deps/openssl/openssl/Configure LIST`. This also contains to add two new defines for all platform in the bottom for GOST and Padlock engines are not included in iojs.

This Makefile executes ../openssl/Configure {target} and copy generated `../openssl/crypto/opensslconf.h` into `archs/{target}/opensslconf.h`. with the target list that is defined in ARCS. Several files are copied for backup and cleared so as not to change the files in `../openssl`. This also applies four fixes as below by using perl script so as to meet iojs build requirements. - all architectures Remove define of OPENSSL_CPUID_OBJ because it is defined in openssl.gypi so as to avoid build error with --openssl-no-asm - VC-WIN32 and VC-WIN64A OPENSSL_NO_DYNAMIC_ENGINE is needed for building static library. OPENSSL_NO_CAPIENG is needed to avoid build errors on Win. See the comments in `deps/openssl/openssl/engines/e_capi.c` for detail. - linux-x32 This fix was made by comparing define values of opensslconf.h which was generated `Configure linux-x32' in openssl-1.0.2a - darwin64-x86_64-cc The current openssl release does not use RC4 asm since it explicitly specified as `$asm=~s/rc4\-[^:]+//;` in https://github.com/openssl/openssl/blob/OpenSSL_1_0_1-stable/Configure#L584 But iojs has used RC4 asm on MacOS for long time. Fix type of RC4_INT into `unsigned int` in opensslconf.h of darwin64-x86_64-cc to work on the RC4 asm.

In OpenSSL, opensslconf.h was generated by Configure script with specifying a target argument, where it includes several defines that depend on OS and architecture platform. In iojs, we statically mapped --dest-os and --dest-cpu options in configure to the target of Configure in OpenSSL and make `deps/openssl/conf/openssconf.h` so as to include each file according to its target by checking pre-defined compiler macros. Included opnesslconf.h files for supported target architectures can be generated by `Makefile` and stored under `archs/{target}/opensslconf.h`. The Makefile alos fixes several defines to meet iojs build requirements. Here is a map table of configure options in iojs, target arch of Configure in OpenSSL and CI support. | --dest-os | --dest-cpu | OpenSSL target arch | CI | | --------- | ---------- | -------------------- | --- | | linux | ia32 | linux-elf | o | | linux | x32 | patched linux-x86_64 | - | | linux | x64 | linux-x86_64 | o | | linux | arm | linux-armv4 | o | | linux | arm64 | N/A | - | | mac | ia32 | darwin-i386-cc | o | | mac | x64 | darwin64-x86-cc | o | | win | ia32 | VC-WIN32 | - | | win | x64 | VC-WIN64A | o | | solaris | ia32 | solaris-x86-gcc | o | | solaris | x64 | solaris64-x86_64-gcc | o | | freebsd | ia32 | BSD-x86 | o | | freebsd | x64 | BSD-x86_64 | o | | openbsd | ia32 | BSD-x86 | - | | openbsd | x64 | BSD-x86_64 | - | | others | others | linux-elf | - | --dest-os and --dest-cpu are mapped to pre-defined macros. | --dest-os | pre-defined macro | | ------------------ | ------------------------- | | win | _WIN32 | | win(64bit) | _WIN64 | | mac | __APPLE__ && __MACH__ | | solaris | __sun | | freebsd | __FreeBSD__ | | openbsd | __OpenBSD__ | | linux (not andorid)| __linux__ && !__ANDROID__ | | android | __ANDROID__ | | --dest-cpu | pre-defined macro | | ---------- | ----------------- | | arm | __arm__ | | arm64 | __aarch64__ | | ia32 | __i386__ | | ia32(win) | _M_IX86 | | mips | __mips__ | | mipsel | __MIPSEL__ | | x32 | __ILP32__ | | x64 | __x86_64__ | | x64(win) | _M_X64 | These are the list which is not implemented yet. | --dest-os | --dest-cpu | OpenSSL target arch | CI | | --------- | ---------- | -------------------- | --- | | linux | mips | linux-mips32,linux-mips64,linux64-mips64? | --- | | linux | mipsel | ? | --- | | android | ia32 | android-x86 | --- | | android | arm | android-armv7 | --- | | android | mips | android-mips | --- | | android | mipsel | ? | --- | Supported target arch list in OpenSSL can be obtained by typing `deps/openssl/openssl/Configure LIST`. This also contains to add two new defines for all platform in the bottom for GOST and Padlock engines are not included in iojs. PR-URL: #1377 Reviewed-By: Fedor Indutny <fedor@indutny.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

This Makefile executes ../openssl/Configure {target} and copy generated `../openssl/crypto/opensslconf.h` into `archs/{target}/opensslconf.h`. with the target list that is defined in ARCS. Several files are copied for backup and cleared so as not to change the files in `../openssl`. This also applies four fixes as below by using perl script so as to meet iojs build requirements. - all architectures Remove define of OPENSSL_CPUID_OBJ because it is defined in openssl.gypi so as to avoid build error with --openssl-no-asm - VC-WIN32 and VC-WIN64A OPENSSL_NO_DYNAMIC_ENGINE is needed for building static library. OPENSSL_NO_CAPIENG is needed to avoid build errors on Win. See the comments in `deps/openssl/openssl/engines/e_capi.c` for detail. - linux-x32 This fix was made by comparing define values of opensslconf.h which was generated `Configure linux-x32' in openssl-1.0.2a - darwin64-x86_64-cc The current openssl release does not use RC4 asm since it explicitly specified as `$asm=~s/rc4\-[^:]+//;` in https://github.com/openssl/openssl/blob/OpenSSL_1_0_1-stable/Configure#L584 But iojs has used RC4 asm on MacOS for long time. Fix type of RC4_INT into `unsigned int` in opensslconf.h of darwin64-x86_64-cc to work on the RC4 asm. PR-URL: #1377 Reviewed-By: Fedor Indutny <fedor@indutny.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

PR-URL: #1377 Reviewed-By: Fedor Indutny <fedor@indutny.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

shigeki · 2015-04-10T01:00:52Z

Thanks, Ben. I fixed the spell of the commit log with my shame. Landed in 29a3301 , 7d14aa0 , 5b0e575 and 8bc8bd4.

mscdex added the openssl Issues and PRs related to the OpenSSL dependency. label Apr 8, 2015

bnoordhuis reviewed Apr 8, 2015
View reviewed changes

shigeki force-pushed the opensslconf branch from 2b11c67 to 1c5db64 Compare April 9, 2015 09:39

shigeki force-pushed the opensslconf branch from 1c5db64 to 16b1848 Compare April 9, 2015 09:49

bnoordhuis reviewed Apr 9, 2015
View reviewed changes

shigeki force-pushed the opensslconf branch 4 times, most recently from 9dd9f17 to 7d1d2b8 Compare April 9, 2015 15:17

Shigeki Ohtsu added 4 commits April 10, 2015 09:42

deps: generate opensslconf.h for architectures

fbfb85f

tools: add to install deps/openssl/config/archs

cb4134d

shigeki force-pushed the opensslconf branch from 7d1d2b8 to cb4134d Compare April 10, 2015 00:49

shigeki pushed a commit that referenced this pull request Apr 10, 2015

deps: generate opensslconf.h for architectures

5b0e575

PR-URL: #1377 Reviewed-By: Fedor Indutny <fedor@indutny.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

shigeki pushed a commit that referenced this pull request Apr 10, 2015

tools: add to install deps/openssl/config/archs

8bc8bd4

PR-URL: #1377 Reviewed-By: Fedor Indutny <fedor@indutny.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

shigeki closed this Apr 10, 2015

rvagg mentioned this pull request Apr 11, 2015

Release proposal: v1.7.0 #1396

Closed

Uh oh!

Make opensslconf.h include depending on target archs #1377

Make opensslconf.h include depending on target archs #1377

Uh oh!

Conversation

shigeki commented Apr 8, 2015

Uh oh!

bnoordhuis commented Apr 8, 2015

Uh oh!

indutny commented Apr 8, 2015

Uh oh!

bnoordhuis Apr 8, 2015

Choose a reason for hiding this comment

Uh oh!

shigeki Apr 9, 2015

Choose a reason for hiding this comment

Uh oh!

shigeki commented Apr 9, 2015

Uh oh!

shigeki commented Apr 9, 2015

Uh oh!

bnoordhuis Apr 9, 2015

Choose a reason for hiding this comment

Uh oh!

shigeki Apr 9, 2015

Choose a reason for hiding this comment

Uh oh!

indutny commented Apr 9, 2015

Uh oh!

shigeki commented Apr 9, 2015

Uh oh!

bnoordhuis Apr 9, 2015

Choose a reason for hiding this comment

Uh oh!

bnoordhuis Apr 9, 2015

Choose a reason for hiding this comment

Uh oh!

shigeki Apr 9, 2015

Choose a reason for hiding this comment

Uh oh!

shigeki commented Apr 9, 2015

Uh oh!

bnoordhuis commented Apr 9, 2015

Uh oh!

shigeki commented Apr 10, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants