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

[vcpkg_find_acquire_program] Split out program data #30780

Merged
merged 1 commit into from
Apr 14, 2023

Conversation

dg0yt
Copy link
Contributor

@dg0yt dg0yt commented Apr 11, 2023

For #30483 (comment):
Allow changing data for a single tool without rebuilding all users of vcpkg_find_acquire_program.
Effects on ABI hashing:

  • unchanged: vcpkg_find_acquire_program.cmake is input to the ABI hash for direct uses of the function, but not for indirect uses (e.g. via vcpkg_fixup_pkgconfig).
  • new: The tool data scripts become input to ABI hashes for direct usage.
  • limitation: The usage must match the data script name literally for the tool data to become part of ABI hashes. (No quotes, no space, no variable.)
  • limitation: Changes to the data for an "interpreter" (PYTHON3, PERL) are no longer input to the ABI hash if used transitively (GASPREPROCESSOR, MESON).

I think the limitations shouldn't be overrated: Most indirect uses weren't covered already before this change (cf. first point). ATM all uses in the repo match literally.

Example for libpcap:

...
vcpkg_find_acquire_program f478cbe84a142e425e7d8111760c1d0a8fcbeacf96762e3c627ad50daec88ffa
vcpkg_find_acquire_program(BISON) 4df1053ab129f7b9ccd5d58baaa91628c9269e259a7239c7eaed795ff1cae1d1
vcpkg_find_acquire_program(FLEX) 5126699579bda4fb2b605adedd8cfed7ac53430178931ffd20d8b007cc89f26f
...

CC @BillyONeal

@Cheney-W Cheney-W added the category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly label Apr 12, 2023
@BillyONeal
Copy link
Member

I like this direction. Will include it in presentation to other maintainers on where vcpkg_find_acquire_program goes.

You've already done awesome stuff, so don't really want to ask you to do more things, but if you're interested you could change the ones that vcpkg fetch already understands to invoke that instead. (But I would merge this myself before doing that if it's up to me)

@dg0yt dg0yt mentioned this pull request Apr 12, 2023
7 tasks
@dg0yt
Copy link
Contributor Author

dg0yt commented Apr 12, 2023

This PR doesn't change what is done at runtime, it just distibutes it differently over the script files.
Once this is merged, it is cheaper to change what is done at runtime (e.g. vcpkg fetch) for individual tools.
That's why I wouldn't want to add more changes ATM, not even the osx 7zip thing.

@dg0yt
Copy link
Contributor Author

dg0yt commented Apr 12, 2023

(In particular, I do not want to trigger more world rebuilds than needed. Tired of fixing baseline regressions.)

@BillyONeal
Copy link
Member

I will force merge this around baseline issues if I get support for the proposal in this direction, so don't lose sleep over that

@BillyONeal BillyONeal merged commit b080256 into microsoft:master Apr 14, 2023
@BillyONeal
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants