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

feat: upgrade to v8 11.6 #66

Merged
merged 10 commits into from
Mar 5, 2024
Merged

Conversation

gengjiawen
Copy link
Collaborator

@gengjiawen gengjiawen commented Jun 20, 2023

fix #65
fix #42

@gengjiawen gengjiawen changed the title feat: initial 11.6 feat: upgrade to v8 11.6 Jun 20, 2023
@bnoordhuis
Copy link
Owner

"The system cannot execute the specified program."

But it's not saying what program and I can't really divine it from the logs... I'm guessing it's builtins-generated from cmake/GenerateBuiltinsList.cmake but not 100% sure.

@gengjiawen
Copy link
Collaborator Author

"The system cannot execute the specified program."

But it's not saying what program and I can't really divine it from the logs... I'm guessing it's builtins-generated from cmake/GenerateBuiltinsList.cmake but not 100% sure.

More like the long path issue on windows. Need a new way to handle torque.

@bnoordhuis
Copy link
Owner

Long path or long argument list? torque.exe ends up D:/a/v8-cmake/build, right?

@gengjiawen
Copy link
Collaborator Author

Long path or long argument list? torque.exe ends up D:/a/v8-cmake/build, right?

long argument list

@bnoordhuis
Copy link
Owner

That's what I thought. CMake has a bunch of response file variables (like CMAKE_CXX_USE_RESPONSE_FILE_FOR_OBJECTS) to get around invoking the compiler or linker with long argument lists but nothing (as far as I know) for using response files with arbitrary executables.

Torque doesn't support reading from response files but that's easy to add.

@bnoordhuis
Copy link
Owner

Forgot to mention the obvious: invoking torque multiple times with shorter argument lists - but I don't know if that actually works. Easy enough to test though.

@bnoordhuis bnoordhuis mentioned this pull request Dec 24, 2023
@barracuda156
Copy link
Contributor

barracuda156 commented Dec 24, 2023

Edited: ah, I did not notice, it has been on CI at least.

I will give it a go locally then.

@gengjiawen
Copy link
Collaborator Author

Edited: ah, I did not notice, it has been on CI at least.

I will give it a go locally then.

macOS and linux should works. windows still has the path issues.

@barracuda156
Copy link
Contributor

@gengjiawen Great.

P. S. Then I just need to find someone who can help sorting out powerpc assembler a bit :)

I have 8.3.110.13 building on macOS ppc, but a) something still needs to be fixed for it to work correctly and b) from 8.4.x+ upstream has broken ppc somewhat further.

@gengjiawen
Copy link
Collaborator Author

on a windows enable long path, build works
CleanShot 2023-12-25 at 14 15 21@2x

@bnoordhuis
Copy link
Owner

How did you get around the long argument list issue?

FWIW, the internet suggests long paths are enabled on GHA Windows runners but git needs to opt in, presumably like so:

diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
index 89498826..cfed0378 100644
--- a/.github/workflows/ci.yml
+++ b/.github/workflows/ci.yml
@@ -18,7 +18,9 @@ jobs:
     steps:
       - uses: actions/checkout@v2
       - name: setup
-        run: cmake -E make_directory ${{runner.workspace}}/build
+        run: |
+          git config core.longpaths true
+          cmake -E make_directory ${{runner.workspace}}/build
       - name: configure
         run: cmake ${{runner.workspace}}/v8-cmake
         working-directory: ${{runner.workspace}}/build

Maybe cmake needs to opt in too, somehow.

@barracuda156
Copy link
Contributor

barracuda156 commented Dec 25, 2023

How did you get around the long argument list issue?

Maybe cmake needs to opt in too, somehow.

Kinda on a side-note, but on macOS long paths can also be a problem. Example: AlphaSparse/Library#19

@gengjiawen
Copy link
Collaborator Author

How did you get around the long argument list issue?

I turned on long-path support on my windows. Not sure if it's GHA Windows revert long path support ?

@bnoordhuis
Copy link
Owner

Hm, still failing with this:

C:\Program Files\Microsoft Visual Studio\2022\Enterprise\MSBuild\Microsoft\VC\v170\Microsoft.CppCommon.targets(254,5): error MSB8066: Custom build for 'D:\a\v8-cmake\build\CMakeFiles\3caf4c045984d86c8d0e2deebc748fc9\builtins.rule;D:\a\v8-cmake\build\CMakeFiles\a8f7d1923a331b4b50796a7a07bdcbe8\aggregate-error-tq-csa.cc.rule;D:\a\v8-cmake\build\CMakeFiles\184467c103758efcc2c8ca2fe9d02f8b\builtins-generated.rule;D:\a\v8-cmake\build\CMakeFiles\d3ae68cc38a20a9bf244124ff4705ebf\bytecodes-builtins-list.h.rule;D:\a\v8-cmake\v8-cmake\CMakeLists.txt' exited with code 9020. [D:\a\v8-cmake\build\v8_torque_generated.vcxproj]

Unclear what exit code 9020 signifies.

@gengjiawen
Copy link
Collaborator Author

Looks GA windows not enabled long path anymore, at least now. Screenshot from runs-on: windows-2022

CleanShot 2023-12-27 at 10 29 54@2x

@gengjiawen
Copy link
Collaborator Author

Can this done per file (sacrifice perf) on windows ?

@bnoordhuis
Copy link
Owner

Hm, that's weird. https://github.com/actions/runner-images/blob/62a46d0fd8/images/windows/scripts/build/Configure-BaseImage.ps1#L73 suggests it's (still?) enabled on 2022 runners. It was definitely enabled on 2019 runners.

@gengjiawen
Copy link
Collaborator Author

Looks still a messy for windows 2019. They break it for somehow.

@gengjiawen
Copy link
Collaborator Author

@bnoordhuis The problem part is torque_outputs is too long, I tried do it in foreach, but still failed. Any suggestions ?

add_custom_command(
  COMMAND
    torque
    -o ${PROJECT_BINARY_DIR}/torque-generated
    -v8-root ${PROJECT_SOURCE_DIR}/v8
    ${torque_files}
  DEPENDS
    torque
    ${torque_dirs}
    ${torque_files_abs}
  OUTPUT
    ${torque-outputs}
    ${torque_outputs}
)

foreach(file IN LISTS torque_outputs)
  add_custom_command(
    OUTPUT ${file}
    COMMAND ${CMAKE_COMMAND} -E touch ${file}
    DEPENDS
      torque
      ${torque_dirs}
      ${torque_files_abs}
  )
endforeach()

@bnoordhuis
Copy link
Owner

The problem part is torque_outputs is too long

How do you figure? I went through the CI results for the last four commits but I'm not sure how you came to that conclusion. I do see a lot of warnings like these:

C:\Program Files\Microsoft Visual Studio\2022\Enterprise\MSBuild\Microsoft\VC\v170\Microsoft.CppCommon.targets(254,5): warning MSB8065: Custom build for item "D:\a\v8-cmake\build\CMakeFiles\a8f7d1923a331b4b50796a7a07bdcbe8\aggregate-error-tq-csa.cc.rule" succeeded, but specified output "d:\a\v8-cmake\build\torque-generated\objects-printer-tq.cc" has not been created. This may cause incremental build to work incorrectly.

Culminating - eventually - in this error:

c1xx : fatal error C1083: Cannot open source file: 'D:\a\v8-cmake\build\torque-generated\objects-printer-tq.cc': No such file or directory [D:\a\v8-cmake\build\v8_torque_generated.vcxproj]

@gengjiawen
Copy link
Collaborator Author

The problem part is torque_outputs is too long

How do you figure? I went through the CI results for the last four commits but I'm not sure how you came to that conclusion. I do see a lot of warnings like these:

Normally win build failed in 2m. When I skip torque_outputs, it failed at 29mins.
https://github.com/bnoordhuis/v8-cmake/actions/runs/7406169157/job/20150198402?pr=66

@bnoordhuis
Copy link
Owner

@gengjiawen can you rebase on top of master? I just merged a commit that - with some luck - fixes the Windows build.

@gengjiawen gengjiawen marked this pull request as ready for review March 4, 2024 12:42
@gengjiawen
Copy link
Collaborator Author

@bnoordhuis Looks like build passed!

@gengjiawen
Copy link
Collaborator Author

@bnoordhuis Let's merge this ?

@bnoordhuis bnoordhuis merged commit bc3893c into bnoordhuis:master Mar 5, 2024
4 checks passed
@bnoordhuis
Copy link
Owner

Yes sorry, lots of things going on at the moment :)

@gengjiawen gengjiawen deleted the feat/11.6 branch March 5, 2024 11:03
@barracuda156
Copy link
Contributor

@bnoordhuis Could this be released in tags?

@bnoordhuis
Copy link
Owner

Done: https://github.com/bnoordhuis/v8-cmake/tree/11.6.189.4

@barracuda156
Copy link
Contributor

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade to V8 11.6 Generating builtins-generated => The system cannot execute the specified program.
3 participants