Skip to content

[vcpkg-tool-ninja] Add only the ninja port#23911

Merged
BillyONeal merged 14 commits intomicrosoft:masterfrom
Neumann-A:add_ninja_port
May 9, 2022
Merged

[vcpkg-tool-ninja] Add only the ninja port#23911
BillyONeal merged 14 commits intomicrosoft:masterfrom
Neumann-A:add_ninja_port

Conversation

@Neumann-A
Copy link
Contributor

@Neumann-A Neumann-A commented Mar 31, 2022

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

You have modified or added at least one portfile where deprecated functions are used.

Details

If you feel able to do so, please consider migrating them to the new functions:
vcpkg_install_cmake -> vcpkg_cmake_install (from port vcpkg-cmake)
vcpkg_build_cmake -> vcpkg_cmake_build (from port vcpkg-cmake)
vcpkg_configure_cmake -> vcpkg_cmake_configure (Please remove the option PREFER_NINJA) (from port vcpkg-cmake)
vcpkg_fixup_cmake_targets -> vcpkg_cmake_config_fixup (from port vcpkg-cmake-config)

In the ports that use the new function, you have to add the corresponding dependencies:

{
  "name": "vcpkg-cmake",
  "host": true
},
{
  "name": "vcpkg-cmake-config",
  "host": true
}

The following files are affected:

  • ports/vcpkg-tool-ninja/portfile.cmake

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

You have modified or added at least one portfile where deprecated functions are used.

Details

If you feel able to do so, please consider migrating them to the new functions:
vcpkg_install_cmake -> vcpkg_cmake_install (from port vcpkg-cmake)
vcpkg_build_cmake -> vcpkg_cmake_build (from port vcpkg-cmake)
vcpkg_configure_cmake -> vcpkg_cmake_configure (Please remove the option PREFER_NINJA) (from port vcpkg-cmake)
vcpkg_fixup_cmake_targets -> vcpkg_cmake_config_fixup (from port vcpkg-cmake-config)

In the ports that use the new function, you have to add the corresponding dependencies:

{
  "name": "vcpkg-cmake",
  "host": true
},
{
  "name": "vcpkg-cmake-config",
  "host": true
}

The following files are affected:

  • ports/vcpkg-tool-ninja/portfile.cmake

@JackBoosY JackBoosY added category:new-port The issue is requesting a new library to be added; consider making a PR! requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. labels Apr 1, 2022
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

You have modified or added at least one portfile where deprecated functions are used.

Details

If you feel able to do so, please consider migrating them to the new functions:
vcpkg_install_cmake -> vcpkg_cmake_install (from port vcpkg-cmake)
vcpkg_build_cmake -> vcpkg_cmake_build (from port vcpkg-cmake)
vcpkg_configure_cmake -> vcpkg_cmake_configure (Please remove the option PREFER_NINJA) (from port vcpkg-cmake)
vcpkg_fixup_cmake_targets -> vcpkg_cmake_config_fixup (from port vcpkg-cmake-config)

In the ports that use the new function, you have to add the corresponding dependencies:

{
  "name": "vcpkg-cmake",
  "host": true
},
{
  "name": "vcpkg-cmake-config",
  "host": true
}

The following files are affected:

  • ports/vcpkg-tool-ninja/portfile.cmake

@strega-nil-ms
Copy link
Contributor

See PR Neumann-A#6 for my requested changes in diff form

@strega-nil-ms strega-nil-ms added requires:author-response and removed requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. labels Apr 5, 2022
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!

PRs must add only one version and must not modify any published versions

When making any changes to a library, the version or port-version in vcpkg.json or CONTROL must be modified.

Error: Local changes detected for vcpkg-tool-ninja but no changes to version or port version.
-- Version: 2022-03-31
-- Old SHA: f68e5b1ee9a97b2d0b414e767a37ed7a2a63a133
-- New SHA: 0a9071fd10711b762bfa1e50f7314f3dd23d41d9
-- Did you remember to update the version or port version?
-- Pass `--overwrite-version` to bypass this check.
***No files were updated.***

@JackBoosY JackBoosY requested a review from strega-nil-ms April 6, 2022 09:00
@JackBoosY
Copy link
Contributor

@strega-nil-ms Any other suggestions?

@strega-nil-ms
Copy link
Contributor

I'd like one last review from @ras0219, I'll ask him in a bit.

@JackBoosY
Copy link
Contributor

I think we can consider to merge this now, @BillyONeal what's the status?

@Neumann-A
Copy link
Contributor Author

So I tested it. I only need this for #23987

@JackBoosY JackBoosY added the info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. label May 5, 2022
@BillyONeal
Copy link
Member

So I tested it. I only need this for #23987

I thought you also needed this because there was no path short enough to build current qt as well?

@Neumann-A
Copy link
Contributor Author

I thought you also needed this because there was no path short enough to build current qt as well?

that was qtwebengine. Doesn't build with the default paths ci uses which is why CURRENT_BUILDTRESS_DIR is moved in the PR you just merged.. Using this PR doesn't help since python bails due to long paths. Probably need to use pythonw for that to see what fails next. But since cl will also bail there is no urgent need. The only way you benefit from the long path support is if you use a full blown llvm toolchain (or intel icx toolchain -> llvm based) on windows which doesn't suffer from any long path issues. I mean we also discussed this in discord didn't we?

REF 170c387a7461d476523ae29c115a58f16e4d3430
SHA512 75c0f263ad325d14c99c9a1d85e571832407b481271a2733e78183a478f7ecd22d84451fc8d7ce16ab20d641ce040761d7ab266695d66bbac5b2b9a3a29aa521
HEAD_REF master
PATCHES PR2056.diff # Long path support windows
Copy link
Member

Choose a reason for hiding this comment

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

Can you vcpkg_download_distfile this patch so that licensing etc remains clear for such a large patch rather than copying it into the vcpkg repo proper?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

any ideas how to download a stable diff ?

I mean i can download "https://patch-diff.githubusercontent.com/raw/ninja-build/ninja/pull/2056.diff" but that is probably not stable.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

There's precedent for it: https://github.com/microsoft/vcpkg/blob/master/ports/libsrtp/portfile.cmake#L1

There's precedent for the this not being stable:
#23461

@BillyONeal
Copy link
Member

I mean we also discussed this in discord didn't we?

Yeah but after it stopped being the current thing I was looking at I stopped following the details. :)

@BillyONeal BillyONeal closed this May 6, 2022
@BillyONeal BillyONeal reopened this May 6, 2022
@BillyONeal
Copy link
Member

close-and-reopen to kick CLA bot

Copy link
Member

@BillyONeal BillyONeal left a comment

Choose a reason for hiding this comment

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

Will merge after I get a second opinion / double check about that vcpkg-port-config.cmake part.

@Neumann-A
Copy link
Contributor Author

Will merge after I get a second opinion / double check about that vcpkg-port-config.cmake part.

This was requested from your side instead of modifying vcpkg_find_acquire_program (#23201 (comment)) or providing hacks for it to find the internal version. It was adjusted in Neumann-A#6. Might also be used for vcpkg-tool-meson in the future.

@BillyONeal
Copy link
Member

Will merge after I get a second opinion / double check about that vcpkg-port-config.cmake part.

This was requested from your side instead of modifying vcpkg_find_acquire_program (#23201 (comment)) or providing hacks for it to find the internal version. It was adjusted in Neumann-A#6. Might also be used for vcpkg-tool-meson in the future.

Right, I know there was discussion about this, and wanted the folks who asked for things to change here to confirm it was in keeping with that direction.

Thanks for the submission :)

@BillyONeal BillyONeal merged commit 83fc3b7 into microsoft:master May 9, 2022
@Neumann-A Neumann-A deleted the add_ninja_port branch May 9, 2022 21:42
@talregev
Copy link
Contributor

talregev commented Apr 12, 2023

@Neumann-A I know it already merge, but I want to understand. This is mean that it compile ninja without nina when it cannot download the binary?

@Neumann-A
Copy link
Contributor Author

The idea here was to have ninja with long path support. However the default tools dont support that so your are still stuck with short paths and only gained a few more characters in length.

@talregev
Copy link
Contributor

Thank you. I will have an issue about my request here.

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

Labels

category:new-port The issue is requesting a new library to be added; consider making a PR! info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants