Skip to content

refactor: dfx build#328

Merged
hansl merged 11 commits intomasterfrom
build
Jan 24, 2020
Merged

refactor: dfx build#328
hansl merged 11 commits intomasterfrom
build

Conversation

@chenyan-dfinity
Copy link
Contributor

@chenyan-dfinity chenyan-dfinity commented Jan 18, 2020 •
edited
Loading

  • Trace local imports for dependency analysis
  • Show warning from moc
  • Show real error message (stdout) from npm
  • Remove duplicate code, consolidate build error messages, show specific cmd when we get an error.
  • Cargo-like build UI

@chenyan-dfinity chenyan-dfinity marked this pull request as ready for review January 21, 2020 18:44
@chenyan-dfinity chenyan-dfinity requested a review from a team as a code owner January 21, 2020 18:44
hansl
hansl reviewed Jan 21, 2020
View reviewed changes
hansl
hansl reviewed Jan 21, 2020
View reviewed changes
@chenyan-dfinity chenyan-dfinity mentioned this pull request Jan 21, 2020
Merged
@hansl
Copy link
Contributor

hansl commented Jan 23, 2020

I've been trying to build a CanisterBuild structure that would hold most of the build information of a canister. Is this something we could do in this PR?

hansl
hansl reviewed Jan 23, 2020
View reviewed changes
src/dfx/src/commands/build.rs
Ok(())
if !surpress_warning && !output.stderr.is_empty() {
// Cannot use eprintln, because it would interfere with the progress bar.
println!("{}", String::from_utf8_lossy(&output.stderr));
Copy link
Contributor

@hansl hansl Jan 23, 2020

Choose a reason for hiding this comment

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

Don't redirect STDERR into STDOUT.

Copy link
Contributor Author

@chenyan-dfinity chenyan-dfinity Jan 24, 2020

Choose a reason for hiding this comment

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

As commented in the code, output to stderr will interfere with the progress bar UI. What's your suggestion?

Copy link
Contributor

@hansl hansl Jan 24, 2020

Choose a reason for hiding this comment

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

This is fine. Our STDOUT should probably be errors / warnings anyway as these are the only information from a build we care about.

@chenyan-dfinity
Copy link
Contributor Author

chenyan-dfinity commented Jan 24, 2020

I've been trying to build a CanisterBuild structure that would hold most of the build information of a canister. Is this something we could do in this PR?

Not in this PR. We have CanisterInfo and MotokoParams that kind of does this. But I think there are design issues we need to discuss before finalizing on the Builder structure, e.g. Do we want dfx to build modules/packages as well? How to support other languages?

@hansl hansl merged commit efae02b into master Jan 24, 2020
@hansl hansl deleted the build branch January 24, 2020 19:06
dfinity-bot added a commit that referenced this pull request Jan 5, 2021
@dfinity-bot
build: niv common: update 41a40026 -> 2e4b3530
22e05f2 
## Changelog for common:
Branch: master
Commits: [dfinity-lab/common@41a40026...2e4b3530](https://github.com/dfinity-lab/common/compare/41a40026b09f744e2605d0afdd5d42bd677cbf2a...2e4b35303b6c04e45c0d3581992569b4bdde66a6)

* [`6bfb5098`](https://github.com/dfinity-lab/common/commit/6bfb50982ca1856c1ed6a94da5e3a4c6ab7eee0c) Add Gabor's SSH Public Key ([dfinity-lab/common⁠#328](http://r.duckduckgo.com/l/?uddg=https://github.com/dfinity-lab/common/issues/328))
* [`045111d0`](https://github.com/dfinity-lab/common/commit/045111d05eb2d281d52ca6ec6bd3006d9209f58f) sam's ssh key added to accounts.nix ([dfinity-lab/common⁠#338](http://r.duckduckgo.com/l/?uddg=https://github.com/dfinity-lab/common/issues/338))
* [`6ec0b53f`](https://github.com/dfinity-lab/common/commit/6ec0b53fe9780cf4d401515e7619b9c697199eeb) Add Maria's ssh key to accounts.nix ([dfinity-lab/common⁠#339](http://r.duckduckgo.com/l/?uddg=https://github.com/dfinity-lab/common/issues/339))
* [`2e4b3530`](https://github.com/dfinity-lab/common/commit/2e4b35303b6c04e45c0d3581992569b4bdde66a6) Update Islam's SSH key ([dfinity-lab/common⁠#342](http://r.duckduckgo.com/l/?uddg=https://github.com/dfinity-lab/common/issues/342))
mergify bot pushed a commit that referenced this pull request Jan 5, 2021
@dfinity-bot
build: niv common: update 41a40026 -> 2e4b3530 (#1262)
57a7bd9 
## Changelog for common:
Branch: master
Commits: [dfinity-lab/common@41a40026...2e4b3530](https://github.com/dfinity-lab/common/compare/41a40026b09f744e2605d0afdd5d42bd677cbf2a...2e4b35303b6c04e45c0d3581992569b4bdde66a6)

* [`6bfb5098`](https://github.com/dfinity-lab/common/commit/6bfb50982ca1856c1ed6a94da5e3a4c6ab7eee0c) Add Gabor's SSH Public Key ([dfinity-lab/common⁠#328](http://r.duckduckgo.com/l/?uddg=https://github.com/dfinity-lab/common/issues/328))
* [`045111d0`](https://github.com/dfinity-lab/common/commit/045111d05eb2d281d52ca6ec6bd3006d9209f58f) sam's ssh key added to accounts.nix ([dfinity-lab/common⁠#338](http://r.duckduckgo.com/l/?uddg=https://github.com/dfinity-lab/common/issues/338))
* [`6ec0b53f`](https://github.com/dfinity-lab/common/commit/6ec0b53fe9780cf4d401515e7619b9c697199eeb) Add Maria's ssh key to accounts.nix ([dfinity-lab/common⁠#339](http://r.duckduckgo.com/l/?uddg=https://github.com/dfinity-lab/common/issues/339))
* [`2e4b3530`](https://github.com/dfinity-lab/common/commit/2e4b35303b6c04e45c0d3581992569b4bdde66a6) Update Islam's SSH key ([dfinity-lab/common⁠#342](http://r.duckduckgo.com/l/?uddg=https://github.com/dfinity-lab/common/issues/342))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@matthewhammer matthewhammer matthewhammer left review comments

@hansl hansl hansl approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@chenyan-dfinity @hansl @matthewhammer