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.

Sign up for GitHub

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

Jump to bottom

[vcpkg script]WIP: ninja 1.12 (Reprise from #39260) #41980

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

[vcpkg script]WIP: ninja 1.12 (Reprise from #39260) #41980

wants to merge 8 commits into from

Conversation

dg0yt
Copy link
Contributor

@dg0yt dg0yt commented Nov 6, 2024

No description provided.

BillyONeal and others added 3 commits June 12, 2024 17:17
@BillyONeal @xb284524239
Update Ninja to 1.12.
883f01d 
Fixes microsoft#38494

Resurrects microsoft#38538

Co-authored-by: xb284524239 <40262910+xb284524239@users.noreply.github.com>
@BillyONeal
1.12.1
a632c24 
Also fix vcpkgTools.xml
@dg0yt
Merge branch 'ninja-1-12' of https://github.com/BillyONeal/vcpkg
991c848
@jimwang118 jimwang118 changed the title WIP: ninja 1.12 (Reprise from #39260) [vcpkg script]WIP: ninja 1.12 (Reprise from #39260) Nov 6, 2024
@jimwang118 jimwang118 added the category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly label Nov 6, 2024
@dg0yt
Copy link
Contributor Author

dg0yt commented Nov 8, 2024

android: Unrelated baseline regressions.

windows: qt5-webengine

  • legacy, needs python2
  • always was best-effort
  • vendored dependencies (chromium, skia) requiring gn requiring ninja.
  • ninja reaches 29348/33281.
  • Reason for error is unclear
jom: D:\b\qt5-webengine\x64-windows-dbg\src\core\Makefile.gn_run.Debug [run_ninja] Error 1
jom: D:\b\qt5-webengine\x64-windows-dbg\src\core\Makefile.gn_run [debug] Error 2
jom: D:\b\qt5-webengine\x64-windows-dbg\src\core\Makefile [sub-gn_run-pro-make_first] Error 2
jom: D:\b\qt5-webengine\x64-windows-dbg\src\Makefile [sub-core-make_first] Error 2
jom: D:\b\qt5-webengine\x64-windows-dbg\Makefile [sub-src-make_first] Error 2

... proposal: add legacy ninja as a per-port download.

@dg0yt dg0yt mentioned this pull request Nov 12, 2024
Merged
@BillyONeal
Copy link
Member

windows: qt5-webengine

Maybe we should skip it given qt5's status at this point.

@dg0yt
Copy link
Contributor Author

dg0yt commented Nov 19, 2024

windows: qt5-webengine

Maybe we should skip it given qt5's status at this point.

qt 5.15.16 was just released. qt5 will stay for a while, also for targeting older devices which cannot run the latest macOS or android (if only vcpkg would support cross builds for qt5).

@dg0yt
Copy link
Contributor Author

dg0yt commented Nov 19, 2024

And marking qt5-webengine as skip would be a lie, because everybody would get the incompatible NINJA from vcpkg, unless forcefully overriding in the triplet.

Yeah, breaking one of the long-path victims by use of the new version of ninja which improves long-path issues is a nice dilemma.

@BillyONeal
Copy link
Member

BillyONeal commented Nov 19, 2024 •
edited
Loading

qt 5.15.16 was just released. qt5 will stay for a while, also for targeting older devices which cannot run the latest macOS or android (if only vcpkg would support cross builds for qt5).

To be clear, I only mean qt5-webengine, not all of qt5. We are already considering skipping it because the x86-windows run is now exceeding the 48 hour absolute time limit and several other triplets are very close.

@dg0yt dg0yt marked this pull request as ready for review November 22, 2024 13:33
Neumann-A
Neumann-A reviewed Nov 25, 2024
View reviewed changes
scripts/cmake/vcpkg_find_acquire_program(NINJA).cmake
@@ -1,17 +1,29 @@
set(program_name ninja)
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like this should use vcpkg fetch ninja somehow so that the ninja in the executable and the ninja provided by the script are defined to be the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are different before this PR and the same with this PR ;-)

Neumann-A
Neumann-A reviewed Nov 25, 2024
View reviewed changes
scripts/cmake/vcpkg_find_acquire_program(NINJA).cmake Outdated
else()
set(build_arch $ENV{PROCESSOR_ARCHITECTURE})
endif()
if((build_arch MATCHES "^(ARM|arm)64$") OR (build_arch MATCHES "^(ARM|arm)$"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if((build_arch MATCHES "^(ARM|arm)64$") OR (build_arch MATCHES "^(ARM|arm)$"))
if((build_arch MATCHES "^(ARM|arm)64$"))

I don't see how arm64 can be run on arm32

@dg0yt
Copy link
Contributor Author

dg0yt commented Dec 9, 2024

Waiting for #42588.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Neumann-A Neumann-A Neumann-A left review comments

Assignees

@jimwang118 jimwang118

Labels
category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dg0yt @BillyONeal @Neumann-A @jimwang118