[draft/inprogress] Metadata changes #21

fearthecowboy · 2022-03-10T22:49:33Z

No description provided.

gcampbell-msft

Looks good overall, just a couple of notes and questions

ce/amf/contact.ts

ce/amf/installer.ts

ce/artifacts/activation.ts

BillyONeal · 2022-03-12T00:27:05Z

Is this still draft/inprogress?

fearthecowboy · 2022-03-14T13:35:58Z

Yes. I'll just open a PR in the other repo

BillyONeal · 2022-03-14T18:42:08Z

Yes. I'll just open a PR in the other repo

I think it's OK to merge here until the PR over there lands; it's easy to replay over there with git apply.

fearthecowboy · 2022-03-14T18:44:39Z

Either here or there is fine with me -- I'm solving one last thing and it would be ready.

BillyONeal

I'm happy with this conditional on a second review on the shell stuff and confirmation of how the catalog is getting updated. We'll talk about transition plan tomorrow.

BillyONeal · 2022-03-12T00:32:52Z

ce/artifacts/activation.ts


-  /** a collection of 'published locations' from artifacts. useful for msbuild */
-  locations = new Map<string, Uri>();
+      // add any remaining entries from existing environment


Should we warn if this happens? I'm thinking of cases like trying to activate a compiler but doing so from a "developer command prompt" and accidentally getting a horrible mishmash of stuff.

If what happens? There is already an existing variable?

Path, Libpath, Include etc - we're just inserting items into the beginning of the variable.

I dunno what we'd do differently.

For PATH we must merge with what happens from the system. But for something like INCLUDE if that's already set the result will probably be a broken mess?

(To be clear, I'm not saying we would do anything different but it may be worth warning about)

Worth watching to see what happens - My worry would be that with a lot of small embedded libs that want a folder added to libpath, we'd be spewing a lot of warnings that are expected.

Thinking about it, if we're always inserting at the front of paths, I'd think that the stuff at the end shouldn't be problematic unless there was no other matches. But might have an impact on intellisense scanning, etc.

Worth thinking about.

BillyONeal · 2022-03-14T20:23:26Z

test/core/amf-tests.ts

-    strict.equal(doc.settings.tools.get('CC'), 'foo/bar/cl.exe', 'should have a value');
-    strict.equal(doc.settings.tools.get('CXX'), 'bin/baz/cl.exe', 'should have a value');
-    strict.equal(doc.settings.tools.get('Whatever'), 'some/tool/path/foo', 'should have a value');
+    strict.equal(doc.exports.tools.get('CC'), 'foo/bar/cl.exe', 'should have a value');


This is a schema change, right? Is anyone on the hook to change the default catalog?

Yeah. I have an updated copy of the repo locally (and was going to convert it to all JSON).

BillyONeal · 2022-03-14T20:47:51Z

ce/artifacts/activation.ts

 import { Uri } from '../util/uri';
 import { toXml } from '../util/xml';
 import { Artifact } from './artifact';

+
+function generateCmdScript(variables: Dictionary<string>, aliases: Dictionary<string>): string {


I asked for a second opinion on what this is doing with other vcpkg maintainers who have more bash experience.

these (generateXYZ) look good to me, but I'm probably the least experienced with bash :P

ce/artifacts/activation.ts

strega-nil · 2022-03-14T22:04:28Z

ce/artifacts/activation.ts

+function generatePosixScript(variables: Dictionary<string>, aliases: Dictionary<string>): string {
+  return linq.entries(variables).select(([k, v]) => { return v ? `export ${k}="${v}"` : `unset ${k[0]}`; }).join('\n') +
+    '\n' +
+    linq.entries(aliases).select(([k, v]) => { return v ? `${k}() {\n  ${v} $* \n}` : `unset -f ${v} > /dev/null 2>&1`; }).join('\n') +


is there a reason this uses functions instead of alias?

No, not that I can think of. I did it right after powershell, where they did need to be functions. Did you want it to use alias/unalias instead?

I don't mind these being functions, they are more flexible and accept positional parameters if we ever were to need those.

fearthecowboy · 2022-03-16T15:29:03Z

I merged my other branch into this one, both need updates in the registry, and it's easier to keep one branch of that.

…adata-changes

BillyONeal · 2022-03-16T20:05:03Z

ce/artifacts/artifact.ts

@@ -209,43 +204,71 @@ export class Artifact extends ArtifactBase {
    await this.targetLocation.delete({ recursive: true, useTrash: false });
  }

+  matchFilesInArtifact(glob: string) {
+    const results = match(this.allPaths, glob.trim(), { dot: true, cwd: this.targetLocation.fsPath, unescape: true });


Suggested change

const results = match(this.allPaths, glob.trim(), { dot: true, cwd: this.targetLocation.fsPath, unescape: true });

const results = match(this.allPaths, glob, { dot: true, cwd: this.targetLocation.fsPath, unescape: true });

Do we really need this trim? If we do that sounds like a bug.

Well, if someone has spaces around the glob in the artifact, "{ **/*.h }" then you get spaces around the string.

BillyONeal · 2022-03-16T20:08:02Z

ce/util/safeEval.ts

-    return sandbox[response];
-  };
-}
+export function safeEval(code: string, context?: any): any {


I think I missed this before but I am strongly opposed to allowing arbitrary ecmascript (a la eval()) in artifact metadata because this forces any implementation of this stuff to include an entire ecmascript implementation going forward. (I know we have no immediate plans to implement with anything else but other customers like Intellisense may want to have higher level understanding of this, and we may be called upon to create ports of this in the future for CPUs where a version of Node does not exist because we are proposing this to underpin all builds)

I have a validator that reduces that to supporting only comparisons - <value> <comparator> <value> where value is an acceptable literal value or $variable reference. Very small surface area. I will add it tomorrow morning.

…work

BillyONeal · 2022-03-23T00:09:28Z

The merger PR microsoft/vcpkg-tool#428 has landed, can you replay this over there?

fearthecowboy · 2022-03-28T16:59:22Z

Had to refactor a bit more, and we're still testing, but I'll send this over to the other repo as soon as it's ready.

vicroms · 2022-04-01T17:46:10Z

ce/amf/demands.ts

 import { cmdlineToArray, execute } from '../util/exec-cmd';
-import { createSandbox } from '../util/safeEval';
+import { safeEval, valiadateExpression } from '../util/safeEval';


Suggested change

import { safeEval, valiadateExpression } from '../util/safeEval';

import { safeEval, validateExpression } from '../util/safeEval';

fearthecowboy added 4 commits March 7, 2022 10:51

wip

a7ad8af

switch to system/git and improve find

c38cb72

fix exit to work properly

09c524e

added a lot of validators

116df56

fearthecowboy requested a review from a team as a code owner March 10, 2022 22:49

gcampbell-msft approved these changes Mar 11, 2022

View reviewed changes

ce/amf/contact.ts Show resolved Hide resolved

ce/amf/installer.ts Show resolved Hide resolved

ce/artifacts/activation.ts Outdated Show resolved Hide resolved

itodirel requested a review from BillyONeal March 11, 2022 02:10

BillyONeal approved these changes Mar 14, 2022

View reviewed changes

vicroms reviewed Mar 14, 2022

View reviewed changes

ce/artifacts/activation.ts Outdated Show resolved Hide resolved

strega-nil reviewed Mar 14, 2022

View reviewed changes

fearthecowboy added 2 commits March 16, 2022 07:44

Merge branch 'fix-search' into metadata-changes

8b9e32c

finish support for braces (validated) expressions

ff2dcec

fearthecowboy added 2 commits March 16, 2022 09:50

Merge branch 'main' of https://github.com/microsoft/vcpkg-ce into met…

7154a74

…adata-changes

fix versions and tweak installer

45cfd1d

BillyONeal reviewed Mar 16, 2022

View reviewed changes

fearthecowboy and others added 7 commits March 17, 2022 15:50

added expression validation and bug fixes

0f3a769

Re-implementing esp-idf install without activation, with Garrett S's …

b034f41

…work

cleaned up

20dd906

removed unused line

a1334a6

saving state of current attempt at activation

8610887

saving state

beee0a6

missed test

e5a253b

WIP - espressif idf working

dd23f61

fearthecowboy added 2 commits March 28, 2022 10:15

Refinements during testing

30d9769

merged from espidf branch

81918c4

vicroms reviewed Apr 1, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[draft/inprogress] Metadata changes #21

[draft/inprogress] Metadata changes #21

fearthecowboy commented Mar 10, 2022

gcampbell-msft left a comment

BillyONeal commented Mar 12, 2022

fearthecowboy commented Mar 14, 2022

BillyONeal commented Mar 14, 2022

fearthecowboy commented Mar 14, 2022

BillyONeal left a comment

BillyONeal Mar 12, 2022

fearthecowboy Mar 14, 2022

BillyONeal Mar 14, 2022

BillyONeal Mar 14, 2022

fearthecowboy Mar 16, 2022

fearthecowboy Mar 16, 2022

BillyONeal Mar 14, 2022

fearthecowboy Mar 14, 2022

BillyONeal Mar 14, 2022

vicroms Mar 14, 2022 •

edited

Loading

strega-nil Mar 14, 2022

fearthecowboy Mar 14, 2022

vicroms Mar 15, 2022

fearthecowboy commented Mar 16, 2022

BillyONeal Mar 16, 2022

fearthecowboy Mar 16, 2022

BillyONeal Mar 16, 2022

fearthecowboy Mar 16, 2022

BillyONeal commented Mar 23, 2022

fearthecowboy commented Mar 28, 2022

vicroms Apr 1, 2022

	const results = match(this.allPaths, glob.trim(), { dot: true, cwd: this.targetLocation.fsPath, unescape: true });
	const results = match(this.allPaths, glob, { dot: true, cwd: this.targetLocation.fsPath, unescape: true });

	import { safeEval, valiadateExpression } from '../util/safeEval';
	import { safeEval, validateExpression } from '../util/safeEval';

[draft/inprogress] Metadata changes #21

Are you sure you want to change the base?

[draft/inprogress] Metadata changes #21

Conversation

fearthecowboy commented Mar 10, 2022

gcampbell-msft left a comment

Choose a reason for hiding this comment

BillyONeal commented Mar 12, 2022

fearthecowboy commented Mar 14, 2022

BillyONeal commented Mar 14, 2022

fearthecowboy commented Mar 14, 2022

BillyONeal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vicroms Mar 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fearthecowboy commented Mar 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BillyONeal commented Mar 23, 2022

fearthecowboy commented Mar 28, 2022

Choose a reason for hiding this comment

vicroms Mar 14, 2022 •

edited

Loading