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

[artifacts] Make various roots consistent between vcpkg and artifacts. #610

Merged
merged 7 commits into from
Jul 13, 2022

Conversation

BillyONeal
Copy link
Member

This is the first chunk of work for https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1494960 "[vcpkg ce] vcpkg-ce does not find vcpkg.json to find vcpkg-configuration.json"

In keeping with #601 (comment) , this adds new parameters to ce for each of the directories in VcpkgPaths that ce cares about, and wires the C++ code to add parameters filling in those values as needed. TypeScript world keeps fallbacks in place but the vast majority of the time we expect the wrapping exe to be used.

Other changes:

  • vcpkg-configuration.json is now the only accepted profile name.
  • vcpkg-configuration.global.json is now replaced with $VCPKG_ROOT/vcpkg-configuration.json, which is consistent with what vcpkg does for manifest-less / classic mode operation.
  • In several places, "cache" is renamed to "downloads" for consistency with the existing naming elsewhere, and also to avoid confusion with "binary caching" or "artifact caching".
  • Implemented "deactivate".
  • Updated the scripts tree sha to one that includes the node 16.15.1 update ( microsoft/vcpkg@032d9d0 ) which drops the need for the --harmony switch.
  • Changes handling of X_VCPKG_REGISTRIES_CACHE to be consistent with all other input paths. This means there is a new intentionally-undocumented-for-now command line switch x-registries-cache .
  • Fixes VS Code to create new files with LF extensions by default.

This is the first chunk of work for https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1494960 "[vcpkg ce] vcpkg-ce does not find vcpkg.json to find vcpkg-configuration.json"

In keeping with microsoft#601 (comment) , this adds new parameters to ce for each of the directories in VcpkgPaths that ce cares about, and wires the C++ code to add parameters filling in those values as needed. TypeScript world keeps fallbacks in place but the vast majority of the time we expect the wrapping exe to be used.

Other changes:
* vcpkg-configuration.json is now the only accepted profile name.
* vcpkg-configuration.global.json is now replaced with $VCPKG_ROOT/vcpkg-configuration.json, which is consistent with what vcpkg does for manifest-less / classic mode operation.
* In several places, "cache" is renamed to "downloads" for consistency with the existing naming elsewhere, and also to avoid confusion with "binary caching" or "artifact caching".
* Implemented "deactivate".
* Updated the scripts tree sha to one that includes the node 16.15.1 update ( microsoft/vcpkg@032d9d0 ) which drops the need for the `--harmony` switch.
* Changes handling of X_VCPKG_REGISTRIES_CACHE to be consistent with all other input paths. This means there is a new intentionally-undocumented-for-now command line switch x-registries-cache .
* Fixes VS Code to create new files with LF extensions by default.
@BillyONeal BillyONeal requested review from ras0219-msft and fearthecowboy and removed request for ras0219-msft June 29, 2022 22:29
@BillyONeal
Copy link
Member Author

I believe this also fixes the problem where artifacts tried to write to vcpkg_root even when it's bundleconfig'd to be readonly

ret = get_platform_cache_home().value_or_exit(VCPKG_LINE_INFO) / "vcpkg" / "registries";
}

return fs.almost_canonical(ret, VCPKG_LINE_INFO);
Copy link
Member

Choose a reason for hiding this comment

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

We lost all the helpful error messages for X_VCPKG_REGISTRIES_CACHE :'(

Copy link
Member Author

Choose a reason for hiding this comment

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

We lost specific messages but I think most all of those already get emitted from the shared bits we're reusing now.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be useful to retain the check that X_VCPKG_REGISTRIES_CACHE must be an absolute path.

Copy link
Member Author

Choose a reason for hiding this comment

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

Why though? This just fixes it with almost_canonical.

Copy link
Member Author

Choose a reason for hiding this comment

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

(Also note now that there's a command line parameter for it so relative paths are more likely and reasonable)

To land this for this tool update I put these checks back but I think we should undo them after discussion once you're back.

Copy link
Member

@vicroms vicroms left a comment

Choose a reason for hiding this comment

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

LGTM

ret = get_platform_cache_home().value_or_exit(VCPKG_LINE_INFO) / "vcpkg" / "registries";
}

return fs.almost_canonical(ret, VCPKG_LINE_INFO);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be useful to retain the check that X_VCPKG_REGISTRIES_CACHE must be an absolute path.

cmd_run.string_arg("--z-enable-metrics");
}

cmd_run.string_arg("--vcpkg-root").string_arg(paths.root);
Copy link
Contributor

Choose a reason for hiding this comment

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

While passing all these parameters as cli args is fine, I want to also note that we do already put them into a json doc inside an environment variable for the recursive vcpkg case.

As an aside, that doc should probably be extended with these new properties.

Copy link
Member Author

Choose a reason for hiding this comment

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

That json blob is at the wrong level of abstraction: it's in the command line parser layer, and records what the command line parser inputs were in hopes that the recursive invocation will re-derive the same values. But here we need the TypeScript world to consume the outputs.

Doing CLI args like this is also easier when debugging since you can get everything you need to do your debugging of that stuff without also needing to each the TypeScript debugger to set up all that separate environment stuff.

@BillyONeal BillyONeal merged commit a5ce80a into microsoft:main Jul 13, 2022
@BillyONeal BillyONeal deleted the global-configuration branch July 13, 2022 04:47
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.

3 participants