Move vcpkg-ce development into the vcpkg-tool repo. #428

BillyONeal · 2022-03-10T01:50:21Z

This change obsoletes https://github.com/microsoft/vcpkg-ce , which will be archived shortly after this merges.

ce development did not maintain a linear history but vcpkg-tool does maintain a linear history, so I have not attempted to preserve "blame".

The following files were not copied over:

.git/**/*
.github/**/* (workflows changes merged into pipelines.yaml)
.scripts/verify-pr.yaml
.scripts/signing/**/*
LICENSE.md
SUPPORT.md
test/vcpkg-ce.test.build.log
azure-pipelines.yml

The following files were modified:

.vscode/**/* was placed in the root, had paths rewritten to target the "ce" subdirectory, and a few tasks renamed to indicate that they target the "ce" subset.
CODE_OF_CONDUCT.md/SECURITY.md/SUPPORT.md/PolicheckExclusion.xml were placed in the root.
.gitattributes was merged with the existing one in the root.
.gitignore had a few things explicitly copied into the root but most were discarded.
readme.md had some content merged.

This change obsoletes https://github.com/microsoft/vcpkg-ce , which will be archived shortly after this merges. ce development did not maintain a linear history but vcpkg-tool does maintain a linear history, so I have not attempted to preserve "blame". The following files were not copied over: * `.git/**/*` * `.github/**/*` (workflows changes merged into pipelines.yaml) * `.scripts/verify-pr.yaml` * `.scripts/signing/**/*` * `LICENSE.md` * `test/vcpkg-ce.test.build.log` * `azure-pipelines.yml` The following files were modified: * `.vscode/**/*` was placed in the root, had paths rewritten to target the "ce" subdirectory, and a few tasks renamed to indicate that they target the "ce" subset. * `CODE_OF_CONDUCT.md`/`SECURITY.md`/`SUPPORT.md`/`PolicheckExclusion.xml` were placed in the root. * `.gitattributes` was merged with the existing one in the root. * `.gitignore` had a few things explicitly copied into the root but most were discarded. * `readme.md` had some content merged.

BillyONeal · 2022-03-10T02:34:52Z

https://devdiv.visualstudio.com/DefaultCollection/DevDiv/_build/results?buildId=5862019

SUPPORT.md

ras0219-msft · 2022-03-10T23:31:57Z

Why is yarn.js 155k lines and checked in? Can't we just require users to install yarn via npm the same way they would install all the other dependencies?

ras0219-msft · 2022-03-10T23:34:04Z

ce/ce/LICENSE

@@ -0,0 +1,21 @@
+MIT License


Shouldn't this be at LICENSE instead of ce/ce/LICENSE?

This is a Node/npm artifact; ce is getting emitted as an npm package here so it needs its own copy of the license. (And I didn't think deduping this file was worth digging through npm's guts to tell it to find it somewhere else)

It can get dropped once it's building the static binary. Since there isn't any standalone usage anymore

ras0219-msft

I am strongly against to checking in yarn.ts.

I am 0.75 against checking in an arbitrarily modified tar stream library. I would vastly prefer either assuming the existence of tar on all machines (e.g. not work on Win7) or to use an unmodified version of the library (via npm). If we've made important bugfixes, then those fixes should be submitted upstream. If upstream is unresponsive, we shouldn't be using the library.

Having custom implementations of tar, bzip, gzip, xz, etc and corresponding test files for that is suspect. As long as the test assets are small enough, I'm ok with merging for now under the assertion that we will eliminate this later.

ras0219-msft · 2022-03-10T23:36:26Z

ce/ce/constants.ts

+// Copyright (c) Microsoft Corporation.
+// Licensed under the MIT License.
+
+export const project = 'environment.yaml';


environment.yaml is not a supported file name, iiuc

ras0219-msft · 2022-03-10T23:36:45Z

ce/ce/constants.ts

+export const latestVersion = '*';
+export const vcpkgDownloadFolder = 'VCPKG_DOWNLOADS';
+export const globalConfigurationFile = 'vcpkg-configuration.global.json';
+export const profileNames = ['vcpkg-configuration.json', 'vcpkg-configuration.yaml', 'environment.yaml', 'environment.yml', 'environment.json'];


I believe vcpkg-configuration.json is the only supported filename.

I really don't want to make functionality changes like that in this PR. I think the search behavior being wrong is already tracked by https://devdiv.visualstudio.com/DefaultCollection/DevDiv/_workitems/edit/1494960

ce/ce/fs/acquire.ts

ce/ce/installers/git.ts

ras0219-msft · 2022-03-10T23:40:10Z

ce/custom/tar-stream/package.json

@@ -0,0 +1,58 @@
+{
+  "name": "tar-stream",


I'm really not keen on checking in this entire separate package including its binary test assets.

ce/getting-started.md

BillyONeal · 2022-03-11T02:33:15Z

I am strongly against to checking in yarn.ts.

@fearthecowboy Do you know what this file is in here for? This is also the one that causes problems by tripping over policheck.

I am 0.75 against checking in an arbitrarily modified tar stream library. I would vastly prefer either assuming the existence of tar on all machines (e.g. not work on Win7) or to use an unmodified version of the library (via npm). If we've made important bugfixes, then those fixes should be submitted upstream. If upstream is unresponsive, we shouldn't be using the library.

I don't think this PR is the place to address a problem like this. tar-stream is itself here to force it to get not vulnerable versions of its dependencies, see microsoft/vcpkg-ce#16 / https://github.com/BillyONeal/vcpkg-ce/pull/1

Having custom implementations of tar, bzip, gzip, xz, etc and corresponding test files for that is suspect. As long as the test assets are small enough, I'm ok with merging for now under the assertion that we will eliminate this later.

Again, this is the status quo and is here from when ce was a separate product. I do think soon we should be funneling all extraction to vcpkg.exe but we can't do that as easily as long as the repos are separate since it would create a cycle in the dependencies of repos :)

BillyONeal · 2022-03-11T03:05:42Z

https://devdiv.visualstudio.com/DefaultCollection/DevDiv/_build/results?buildId=5865809&view=results was a successful build before I fixed a few nitpicks here. https://devdiv.visualstudio.com/DefaultCollection/DevDiv/_build/results?buildId=5868553&view=results is a build identical to the commit sha here in progress.

fearthecowboy · 2022-03-11T15:04:54Z

You can remove yarn entirely. It's not necessary - it was used to bootstrap ce by itself without having to deal with npm. That's gone now, so just drop it.

fearthecowboy · 2022-03-11T15:21:19Z

The tar-stream library is checked in because the upstream uses some libraries that replicate the built-in node libraries in later versions of node. The libraries allow the usage under earlier versions of node, but they caused issues with the compliance scanner, and in this case were not only completely unnecessary, but caused a bunch of bloat for no value. Stripping them out fixed the problem. If you want to replace it with calls to native tar, I have no problem with it.

fearthecowboy

I'd remove yarn, since it's not needed for anything here. Past that, I'm good.

BillyONeal · 2022-03-14T19:37:24Z

I'd remove yarn, since it's not needed for anything here. Past that, I'm good.

microsoft/vcpkg-ce#22

…atch what vcpkg expects. Note that the "VCPKG_BASE_VERSION" unset path will be substantially modified after microsoft#428 , this change always rebootstraps CE. (I can see an argument for it to never rebootstrap CE?)

…atch what vcpkg expects. Note that the "VCPKG_BASE_VERSION" unset path will be substantially modified after microsoft#428 , this change always rebootstraps CE. (I can see an argument for it to never rebootstrap CE?) Resolves https://devdiv.visualstudio.com/DefaultCollection/DevDiv/_workitems/edit/1494969

ras0219-msft · 2022-03-17T16:59:49Z

With yarn removed, I ran git archive --output src.zip and looked at the deflated contents.

First, ce/custom/tar-stream/test/fixtures/huge.tar.gz is not terribly large compressed (14 kb) but is 8.5 MB uncompressed. Assuming we are committing to remove tar-stream, I want to minimize the long-term impact we suffer from checking in all its binary files. I'd propose not checking in its test/ directory entirely.

Second, ce/test/resources/2021-05-06-VisualStudio.vsman is 50% of the total compressed size (1.5 MB compressed). It's also 10MB uncompressed, which is over twice as large as the entire current vcpkg-tool repo (4 MB uncompressed). I see that it is a single absolutely enormous JSON document but our tests are covering only a tiny fraction of the total items. Is there strong opposition to trimming the file down to only the relevant entries? I volunteer to do this.

Third, I don't see a single reference to the images in docs/imgs/ -- can these be deleted?

fearthecowboy · 2022-03-17T17:02:49Z

The /docs folder can be dropped imo. I don't think there is anything relevant in there that needs to be kept.

…atch (#440) * Detect changes in the 'ce' version and rebootstrap it if it doesn't match what vcpkg expects. Note that the "VCPKG_BASE_VERSION" unset path will be substantially modified after #428 , this change always rebootstraps CE. (I can see an argument for it to never rebootstrap CE?) Resolves https://devdiv.visualstudio.com/DefaultCollection/DevDiv/_workitems/edit/1494969 * Add msg::print_error and msg::print_warning.

Co-authored-by: Robert Schumacher <roschuma@microsoft.com>

ras0219-msft

Thanks @BillyONeal, that addresses my size concerns.

(Note: I have not reviewed any significant portion of the individual files in this merge.)

* Add back missing .vscode directory lost by .gitignore in #428 * [vcpkg-ce] Fix hash checks. Resolves https://devdiv.visualstudio.com/DefaultCollection/DevDiv/_workitems/edit/1495068 I'm not a fan of the stringly-typed API here allowing this kind of mistake to be silently made but I'm a fan of leaving this broken for a long time while we design something better worse. * Minimize VS Code support files.

BillyONeal added 4 commits March 9, 2022 17:49

Add missing gitignores

1e7e781

Remove broken fast-xml-parser reference.

3cde7f5

Fix some more vcpkg-ce names.

5b48da1

Fix some repo docs.

5fb523d

Neumann-A reviewed Mar 10, 2022

View reviewed changes

SUPPORT.md Outdated Show resolved Hide resolved

Add back overzealous vcpkg- removal for vcpkg-init.

e9a3a33

ras0219-msft reviewed Mar 10, 2022

View reviewed changes

ras0219-msft requested changes Mar 10, 2022

View reviewed changes

Delete support.md as it's irrelevant in the tool repo.

74f6ff1

BillyONeal added 2 commits March 10, 2022 19:04

Merge remote-tracking branch 'origin/main' into merge-ce-repo

25cdcd0

Merge remote-tracking branch 'internal/merge-ce-repo' into merge-ce-repo

f50bf6a

BillyONeal marked this pull request as ready for review March 11, 2022 03:05

fearthecowboy suggested changes Mar 11, 2022

View reviewed changes

Replay microsoft/vcpkg-ce#22

ad3d88f

fearthecowboy approved these changes Mar 16, 2022

View reviewed changes

BillyONeal mentioned this pull request Mar 17, 2022

Detect changes in the 'ce' version and rebootstrap it if it doesn't match #440

Merged

Merge remote-tracking branch 'origin/main' into merge-ce-repo

fe6cf95

BillyONeal and others added 3 commits March 22, 2022 11:50

Remove big test assets.

acd9909

Update ce/getting-started.md

60d2599

Co-authored-by: Robert Schumacher <roschuma@microsoft.com>

Delete 'docs' folder.

3986e81

BillyONeal requested a review from ras0219-msft March 22, 2022 18:53

ras0219-msft approved these changes Mar 22, 2022

View reviewed changes

BillyONeal merged commit e0600cb into microsoft:main Mar 23, 2022

BillyONeal deleted the merge-ce-repo branch March 23, 2022 00:08

BillyONeal mentioned this pull request Mar 23, 2022

[draft/inprogress] Metadata changes microsoft/vcpkg-ce#21

Open

BillyONeal added a commit to BillyONeal/vcpkg-tool that referenced this pull request Mar 23, 2022

Add back missing .vscode directory lost by .gitignore in microsoft#428

b469c89

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move vcpkg-ce development into the vcpkg-tool repo. #428

Move vcpkg-ce development into the vcpkg-tool repo. #428

BillyONeal commented Mar 10, 2022 •

edited

Loading

BillyONeal commented Mar 10, 2022

ras0219-msft commented Mar 10, 2022

ras0219-msft Mar 10, 2022

BillyONeal Mar 11, 2022

fearthecowboy Mar 11, 2022

ras0219-msft left a comment •

edited

Loading

ras0219-msft Mar 10, 2022

ras0219-msft Mar 10, 2022

BillyONeal Mar 11, 2022

ras0219-msft Mar 10, 2022

BillyONeal commented Mar 11, 2022

BillyONeal commented Mar 11, 2022

fearthecowboy commented Mar 11, 2022

fearthecowboy commented Mar 11, 2022

fearthecowboy left a comment

BillyONeal commented Mar 14, 2022

ras0219-msft commented Mar 17, 2022

fearthecowboy commented Mar 17, 2022

ras0219-msft left a comment •

edited

Loading

Move vcpkg-ce development into the vcpkg-tool repo. #428

Move vcpkg-ce development into the vcpkg-tool repo. #428

Conversation

BillyONeal commented Mar 10, 2022 • edited Loading

BillyONeal commented Mar 10, 2022

ras0219-msft commented Mar 10, 2022

ras0219-msft Mar 10, 2022

Choose a reason for hiding this comment

BillyONeal Mar 11, 2022

Choose a reason for hiding this comment

fearthecowboy Mar 11, 2022

Choose a reason for hiding this comment

ras0219-msft left a comment • edited Loading

Choose a reason for hiding this comment

ras0219-msft Mar 10, 2022

Choose a reason for hiding this comment

ras0219-msft Mar 10, 2022

Choose a reason for hiding this comment

BillyONeal Mar 11, 2022

Choose a reason for hiding this comment

ras0219-msft Mar 10, 2022

Choose a reason for hiding this comment

BillyONeal commented Mar 11, 2022

BillyONeal commented Mar 11, 2022

fearthecowboy commented Mar 11, 2022

fearthecowboy commented Mar 11, 2022

fearthecowboy left a comment

Choose a reason for hiding this comment

BillyONeal commented Mar 14, 2022

ras0219-msft commented Mar 17, 2022

fearthecowboy commented Mar 17, 2022

ras0219-msft left a comment • edited Loading

Choose a reason for hiding this comment

BillyONeal commented Mar 10, 2022 •

edited

Loading

ras0219-msft left a comment •

edited

Loading

ras0219-msft left a comment •

edited

Loading