test: rebuild nan tests with libc++ and libc++abi #29281
Conversation
Fixes #28414. I've confirmed this fix wfm on Linux. Pushing into a PR to get CI to run it out on Win and Mac platforms too.
Linting the file adjusted the licenses array, which only contains one value, and causes the gn check to fail later
One final change then I think we're gtg
Co-authored-by: Samuel Attard <[email protected]>
|
No Release Notes |
|
I was unable to backport this PR to "13-x-y" cleanly; |
* test: re-enable nan test: typedarrays-test.js Fixes #28414. I've confirmed this fix wfm on Linux. Pushing into a PR to get CI to run it out on Win and Mac platforms too. * chore: clarify comment * test: fix NAN test string alignment * test: (wip) add ldflags, archive file for libc++ * test: (wip) add libc++ to CircleCI * test: (wip) add llvm flags * test: (wip) change ldflag syntax * test: (wip) build libc++abi as static * fix: correct ldflags * test: add ld env * fix: do not commit this * test: add lld from src to circleci * test: add lld link to ld * chore: preserve third_party * seems legit * sam swears this works kinda sort of sometimes' : * build: add gn visibility patch * chore: update patches * build: check for flatten_relative_to = false * build: upload zip files, add to release.js validation * debug: what the hell gn * build: add libcxx gni to lint ignore Linting the file adjusted the licenses array, which only contains one value, and causes the gn check to fail later * build: also use nan-spec-runner flags on Windows * build: add linked flags for win32 only * build: build libc++ as source on win * build: clean up patch, add -fPIC for IA32 * build: delete libcxx .a files from root * build: rename libc++.zip, clean up upload per platform * build: fix gni lint * ci: add libcxx gen to circleci config * build: correct libcxx-object syntax Co-authored-by: Samuel Attard <[email protected]> Co-authored-by: Charles Kerr <[email protected]> Co-authored-by: clavin <[email protected]> Co-authored-by: Samuel Attard <[email protected]> Co-authored-by: PatchUp <73610968+patchup[bot]@users.noreply.github.com> Co-authored-by: Samuel Attard <[email protected]>
|
@VerteDinde has manually backported this PR to "13-x-y", please check out #29294 |
* test: rebuild nan tests with libc++ and libc++abi (#29281) * test: re-enable nan test: typedarrays-test.js Fixes #28414. I've confirmed this fix wfm on Linux. Pushing into a PR to get CI to run it out on Win and Mac platforms too. * chore: clarify comment * test: fix NAN test string alignment * test: (wip) add ldflags, archive file for libc++ * test: (wip) add libc++ to CircleCI * test: (wip) add llvm flags * test: (wip) change ldflag syntax * test: (wip) build libc++abi as static * fix: correct ldflags * test: add ld env * fix: do not commit this * test: add lld from src to circleci * test: add lld link to ld * chore: preserve third_party * seems legit * sam swears this works kinda sort of sometimes' : * build: add gn visibility patch * chore: update patches * build: check for flatten_relative_to = false * build: upload zip files, add to release.js validation * debug: what the hell gn * build: add libcxx gni to lint ignore Linting the file adjusted the licenses array, which only contains one value, and causes the gn check to fail later * build: also use nan-spec-runner flags on Windows * build: add linked flags for win32 only * build: build libc++ as source on win * build: clean up patch, add -fPIC for IA32 * build: delete libcxx .a files from root * build: rename libc++.zip, clean up upload per platform * build: fix gni lint * ci: add libcxx gen to circleci config * build: correct libcxx-object syntax Co-authored-by: Samuel Attard <[email protected]> Co-authored-by: Charles Kerr <[email protected]> Co-authored-by: clavin <[email protected]> Co-authored-by: Samuel Attard <[email protected]> Co-authored-by: PatchUp <73610968+patchup[bot]@users.noreply.github.com> Co-authored-by: Samuel Attard <[email protected]> * build: correct libcxx_objects build action name * build: only upload libcxx headers on linux * build: ensure object files are included even if unparsable Co-authored-by: Charles Kerr <[email protected]> Co-authored-by: clavin <[email protected]> Co-authored-by: Samuel Attard <[email protected]> Co-authored-by: PatchUp <73610968+patchup[bot]@users.noreply.github.com> Co-authored-by: Samuel Attard <[email protected]> Co-authored-by: Samuel Attard <[email protected]>
| Build libc++ as static library to compile and pass | ||
| nan tests |
Can we expand this description somewhat? In particular, per https://github.com/electron/electron/blob/master/docs/development/patches.md#patch-justification, I'd like to know:
- Do we expect to ever be able to remove this patch? If so, under what circumstances? How will a future maintainer know if it's safe to remove this patch?
- What specific issue does this patch address?
To get yourself in the appropriate mindset, imagine you're an Electron maintainer five years in the future and the people who wrote and reviewed this patch no longer work on the project (and probably don't remember writing this patch at all). What information would you find useful?
This is a very good call out, I'll amend this and make the patch
| Date: Wed, 12 May 2021 12:43:07 -0600 | ||
| Subject: nan_string_test_alignment | ||
|
|
||
| Modifies a NAN test to avoid a debug check pertaining to efficient string alignment. |
- Which DCHECK?
- As above, under what circumstances is it safe to remove this patch? (Can we e.g. upstream it?)
We should actually be able to upstream this fairly easily if the Nan maintainers agree to it! I'll add this to the patch
| // TODO(ckerr) this is cribbed from read obj/electron/electron_app.ninja. | ||
| // Maybe it would be better to have this script literally open up that | ||
| // file and pull cflags_cc from it instead of using bespoke code here? | ||
| // I think it's unlikely to work; but if it does, it would be more futureproof |
IMO no, for the following reasons:
- electron_app.ninja has lots of unrelated flags (e.g. include paths) which could potentially adversely affect this compilation.
- It's useful to know what the minimal set of flags needed is, especially since we will need to mirror those flags over to electron-rebuild.
I'll remove this comment entirely so we don't go down the wrong path in the future
| '-D_LIBCPP_HAS_NO_VENDOR_AVAILABILITY_ANNOTATIONS', // needed by next line | ||
| `-isystem"${path.resolve(BASE, 'buildtools', 'third_party', 'libc++', 'trunk', 'include')}"`, | ||
| `-isystem"${path.resolve(BASE, 'buildtools', 'third_party', 'libc++abi', 'trunk', 'include')}"`, | ||
| '-fPIC' |
hm, isn't -fPIC already passed by other code (e.g. gyp)?
It is on linux x64, but it's not passed by default for linux IA32. This flag was added to pass on IA32, I can look into a better solution
that's a bit surprising. i wonder why this isn't passed on ia32? it looks like it's been that way forever. not that we care terribly much about ia32, but does it break things to pass -fPIC there?
@nornagon It probably doesn't break anything for us, but we can't guarantee that for node and all of it's native modules. Like it clearly works in our ia32 CI but also that isn't technically running on ia32 hardware either
Description of Change
Fixes #28414
Beginning in Electron 13, the nan
typedarrays-test.js testbegan failing due to an incompatibility between libc++ and libstdc++. We now build these tests against libc++ to fix this and prevent future incompatibilities, by copying some of the relevant build arguments from Electron into our nan test runner.In addition to changing the test arguments, we also now build libc++ and libc++abi as static libraries for Mac and Linux. The relevant zipped headers for these libraries are also now checked and uploaded - electron-rebuild will also need to be updated with these build changes, and use the zipped headers to apply them.
References to #25561 and #29028 as a starting point.
Note: This started off as a testing branch, so there are several commits that are just debugging mess. I preserved the git history since we squash and merge anyway, but happy to clean up those commits if that would make reviewing easier.
Checklist
npm testpassesRelease Notes
Notes: none
The text was updated successfully, but these errors were encountered: