Skip to content

update DFX-Interface#1148

Merged
chenyan-dfinity merged 5 commits intomasterfrom
dfx
Jan 25, 2020
Merged

update DFX-Interface#1148
chenyan-dfinity merged 5 commits intomasterfrom
dfx

Conversation

@chenyan-dfinity
Copy link
Contributor

No description provided.

nomeata
nomeata previously approved these changes Jan 21, 2020
@nomeata nomeata dismissed their stale review January 21, 2020 22:06

Didn't scroll down when reading the changes

canister:alias
// Resolved imports
some/path/local_import.mo
some/path/local_import.wasm
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems fragile to mix urls and resolved paths.

Maybe an alternative would be to have multiple flags:

  • --print-canister-imports to only print ic:… and canister… imports. This is what dfx needs to know which aliases to set up and which IDL files to import.
  • --print-read-files to only print the actual files that moc would read (including the resolved IDL files!), for dependency/recompilation tracking.

In fact, currently dfx only needs the former. So maybe we just provide that? And let dfx tell us if they need more? Rather than making maybe not very helpful guesses about what they “might” need.

@hansl, thoughts?

Copy link
Contributor Author

@chenyan-dfinity chenyan-dfinity Jan 21, 2020

Choose a reason for hiding this comment

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

dfx needs to know both. We can import a canister inside a local import file. Not sure what's the best format to present this. Maybe we can output the original URL followed by a full path if moc can resolve it?

btw, I have refactor PR for dfx build, so now we know better what is needed from moc: dfinity/sdk#328

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can output the original URL followed by a full path if moc can resolve it?

Yes, that would work nicely, and is maybe general enough to not change as dfx gets smarter or change its meaning.

Would you still want it to only print the immediate imports, or also open those it can find and print those imprted there (i.e. transitively)?

But now the output gets structural, so bikeshedding about the format ensues. I am sure Andreas will veto JSON touching the Motoko compiler. Simply space separated? S-Expressions?

Copy link
Contributor Author

@chenyan-dfinity chenyan-dfinity Jan 22, 2020

Choose a reason for hiding this comment

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

Transitive imports are definitely better: 1) dfx doesn't need to handle local import errors, and can probably ignore most of the URL prefixes; 2) dfx doesn't need to call moc multiple times.

Is --print-deps only intended to be used by dfx? If so, we only need to output a set of canisters the file transitively import. We can output local file as well for completeness. Then the format is simply a set, not structural. @matthewhammer had a use case earlier that he wanted to see the transitive imports as well.

Space and comma are fragile, as the full path can contain these characters. Maybe we can output two lines for each import, e.g.

mo:stdlib/list
  resolves to: path/stdlib/list.mo
canister:alias
  

The import dependency is a DAG, not a tree. So it's not easy to show the structure in textual format.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is --print-deps only intended to be used by dfx? If so, we only need to output a set of canisters the file transitively import.

Then I’d rather see --print-canister-imports.

Space and comma are fragile, as the full path can contain these characters

No spaces if we output them as urls (e.g. file://), due to URL encoding.

I think below (“two lines”) you are coming up with a space-sensitive ad-hoc format – better use something existing (S-Expressions or JSON, I guess).

Copy link
Contributor Author

@chenyan-dfinity chenyan-dfinity Jan 23, 2020

Choose a reason for hiding this comment

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

Then I’d rather see --print-canister-imports.

It's more robust we output all imports. dfx can use these info to decide if we want to compile only a subset of canisters.

No spaces if we output them as urls (e.g. file://), due to URL encoding.

Nice, if you use file:// then we can simply output in one line with a space in between.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, care to update your proposal?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated. I am a bit hesitated to use file://,

  1. If the import string is a URL, then we don't have spaces in the first part. Then it's fine to use space as a separator, and then just output the resolved full path in the second part.
  2. We need to pull in an extra dependency for moc for encoding file path URL, which is not really used anywhere other than this feature. It also requires the receiver side to decode.

Copy link
Contributor

@nomeata nomeata left a comment

Choose a reason for hiding this comment

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

If this is what dfx needs, then fine with me. I wound’t be surprised if we need to iterate on this a bit more (both syntax and semantics). But we can do that, of course.

@kritzcreek do you want to own implementing this?

@chenyan-dfinity chenyan-dfinity merged commit 6c46355 into master Jan 25, 2020
@chenyan-dfinity chenyan-dfinity deleted the dfx branch January 25, 2020 22:16
@dfinity-ci
Copy link

In terms of size, no changes are observed in 1 tests.
In terms of gas, no changes are observed in 1 tests.

@nomeata
Copy link
Contributor

nomeata commented Jan 25, 2020

Or will you simply build what you need for dfx yourself, @chenyan-dfinity?

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