Replace bash with cargo-make

[sagiegurari]

Description

See #1418
Basically this first draft only replaces the run_stable_checks script to cargo make.
In addition it also installs rustfmt, clippy and cargo-web if missing.
You can run it from the root as cargo make stable-checks

By the way, one of the member crates is failing on cargo check.

[cargo-make][2] INFO - Execute Command: "cargo" "check"
    Checking pure_component v0.1.0 (/workspace/yew/yewtil/examples/pure_component)
error[E0432]: unresolved imports `yewtil::Pure`, `yewtil::PureComponent`
 --> yewtil/examples/pure_component/src/button.rs:3:14
  |
3 | use yewtil::{Pure, PureComponent};
  |              ^^^^  ^^^^^^^^^^^^^ no `PureComponent` in the root
  |              |
  |              no `Pure` in the root

error: aborting due to previous error

Fixes # (issue)

Checklist:

• [ x] I have run ./ci/run_stable_checks.sh. - sort of, i ran cargo make stable-checks
• [ x] I have reviewed my own code
• I have added tests

[teymour-aldridge]

I think the issue is to do with a mismatch with the feature flags.

[teymour-aldridge]

https://github.com/yewstack/yew/blob/master/yewtil/Cargo.toml

[sagiegurari]

thanks, so should use default feature for the check, or should that be fixed in that member?
feels like a fix is more correct?

[siku2]

The pure_components example for yewtil doesn't specify the "pure" feature for some reason. I do believe that should be fixed, yes:

--- a/yewtil/examples/pure_component/Cargo.toml
+++ b/yewtil/examples/pure_component/Cargo.toml
@@ -9,4 +9,4 @@ license = "MIT/Apache-2.0"
 log = "0.4.8"
 wasm-logger = "0.2.0"
 yew = { path = "../../../yew" }
-yewtil = { path = "../.." }
+yewtil = { path = "../..", features = ["pure"] }

[siku2]

This looks great so far! Much more complex than I had anticipated so I can't thank you enough for your help.

One thing that sticks out to me right now is the amount of console output. Running cargo make stable-checks generates a whopping 1500 lines of output (not counting the output generated by rustc). This isn't that big of an issue but I feel like there's a lot of verbosity in there that isn't useful to the everyday contributor.
Is there a way to cut down on the output outside of CI environments? And I don't mean setting the log level to ERROR because that would remove pretty much all of it.

Speaking of the log level, you're running cargo make separately for yew-stdweb which ignores the command line arguments passed to the parent command. Does cargo-make have a mechanism to forward certain options like --loglevel, --profile, and --output-format? I don't think this is a problem for us right now, I'm mostly just curious.

[sagiegurari]

thanks a lot for the feedback. you are raising great points and i'll try to answer.

console logs - i get that alot actually. many people want less. on the other hand, i'm getting the opposite requests, for example there is one open issue now from someone that requested time summary per task (i implemented it but its not on by default) and others wanted to know if a task is skipped than to printout why and so on...
so i'm walking a fine line here...
however, i'll try to see how much i can reduce more 'useless' info.
i do think we need to know what steps are running (not all steps would have an output like build, so you need some feedback that the step happened).
so i'll check what can be done.

cli args - good observation :)
so it really depends on the specific cli arg. many times i convert those to env vars so they are automatically available to child processes, but not always.
there are solutions for that, but i did quick and dirty to have something working.

complexity - i think most complexity comes from the fact that this project has some non standard structure. you have a workspace inside a workspace.
every now and then i find a new setup :) so i try to provide a solution for it.
in your case, it was to split the build to 2 and run each as a child process.
its not great, but i'm not sure what else to do without creating a fake workspace (cargo-make supports workspace emulation, meaning you can define a workspace structure different from the one in the cargo.toml file, i did that for https://github.com/enarx/enarx project that didn't want a rust workspace, but wanted the workspace capability of cargo-make) and that will require to manually defining all members (and you have a lot of them).
if you meant something else, i would be happy to hear and think about how to make things simpler.

[sagiegurari]

@siku2 did more improvements which are based on new cargo-make version which is still in development (will be releasing it this week).
so if you want to test it, install cargo make as follows:

cargo install --force --git https://github.com/sagiegurari/cargo-make --branch 0.32.2

console output - this one should be resolved. i greatly reduced unneeded logs. give it a try and compare

cli args - that duel workspace you have is a bit problematic for me, but i think this one should be ok now. i'm passing the important stuff on and the rest is automatically passed via env.

I'll probably try to tackle more scripts if the overall approach seems relevant for you.

also, i saw in the original issue there was a question if cargo-make is mature. so apart of the fact its there for around 3 years, it is very much in use by many projects.
few big ones i can pointout to are (i took the descriptions from the github pages):

• rustfmt
• seed - Seed is a front-end Rust framework for creating fast and reliable web apps with an elm-like architecture.
• juniper - GraphQL server library for Rust
• trust-dns - A Rust based DNS client, server, and Resolver, built to be safe and secure from the ground up.

And few more
Although this is not exactly a good search as project may have multiple makefiles and they may have 0 makefiles and use the core tasks only.

[siku2]

console output - this one should be resolved. i greatly reduced unneeded logs. give it a try and compare

That's really cool! It seems like we're down to just under 700 lines now. Still a lot, but cutting it in half is a great achievement.
Thanks for taking a look.

that duel workspace you have is a bit problematic for me

Haha, tell me about it. Glad to hear that cargo-make is up to the task.

I'll probably try to tackle more scripts if the overall approach seems relevant for you.

Yes, this looks great so far, please do. The most important script left is run_tests.sh which might pose a challenge because of the httpbin docker container.
After that the only other uesr-facing script is the one for running examples but that will hopefully be done by the yew-cli in the future so it doesn't need to be ported.

I would love to use cargo-make for the entire CI in the future and that will require some additional tasks. I'll gladly do those myself though - wouldn't want to take up too much of your time.

So TL;DR, it would be great if you could add a task for running the tests (run_tests.sh) as well.

[sagiegurari]

I'll take a look at it. thanks for the feedback :)

[sagiegurari]

@siku2 i have implemented (i hope) the run tests as well but need your help testing it fully.
wasm build fail because i don't have a browser installed and since I'm using gitpod, my options are a bit limited.

the new run-tests task will also automatically install wasm binary if needed.

i have excluded all example members from the tests since i see you test very specific members (i took the reverse approach of saying what not to test)

[siku2]

Great, I will take a look and let you know if it's working :)

[siku2]

Okay, it looks like everything is working!

There's one thing left that I'm hoping cargo-make might be able to deal with and that's managing the httpbin Docker container for the integration tests.
Right now it relies on HTTPBIN_URL being set, which can be a bit annoying because it requires the user to manually handle the server either through Docker or other means.

Ideally it should work like this:

run-tests checks for the presence of the HTTPBIN_URL variable, if it isn't present it automatically starts a httpbin container with docker run -p 8080:8080 kennethreitz/httpbin and sets HTTPBIN_URL to "http://localhost:8080".
This should generate an error if Docker isn't available but there should probably be some kind of flag like --skip-httpbin so it can be used without having docker installed.
When the integration tests are done the container should be removed if it was created by cargo-make.

Not sure how much work it is to implement this so do tell me if you don't have the time.
This is already a big step up from what we currently have so I'm willing to merge as is!
From what I've seen so far cargo-make doesn't have a Docker integration

[sagiegurari]

thanks for checking.

so for the docker stuff, it sounds like simple scripting i can add to the makefile.
But i wonder why didn't you add it to the shell script to being with? was there any technical issue?
Also i'm not sure you want to remove the docker container created, maybe stop it, but removing it would make the build slow every time.
But... while i never tested it, i see there is a public http bin docker container running at http://httpbin.org/
why not just use that in case HTTPBIN_URL is undefined (meaning if not defined, default to that unless specified otherwise)?

[siku2]

But i wonder why didn't you add it to the shell script to being with? was there any technical issue?

You have to understand that these scripts are ancient and were only designed for Travis initially. In an earlier version of the script there wasn't even a check for the HTTPBIN_URL variable, it was just hard-coded because the Travis script managed the container.
I never bothered to add more functionality other than some basic safeguards because I want to move away from bash scripts.

Also i'm not sure you want to remove the docker container created, maybe stop it, but removing it would make the build slow every time.

Starting a container from a cached image shouldn't take long at all, especially not compared to the time it takes to run the tests in the first place.
Wouldn't be a deal breaker if the task didn't remove the container though.

But... while i never tested it, i see there is a public http bin docker container running at http://httpbin.org/
why not just use that in case HTTPBIN_URL is undefined (meaning if not defined, default to that unless specified otherwise)?

We used that a while back but it suddenly stopped working correctly. I don't remember what was broken exactly but our tests kept failing.

[sagiegurari]

cool, thanks for the answers. I'll try to see what i can do.

[sagiegurari]

@siku2 I implemented the docker start/stop. it was pretty simple but fully testing it is not possible for me so again i'll have to use your testing skills :)

apart of that, i think this PR is ready for review. I think all the stuff we wanted to push for first iteration is there and ready.

[siku2]

We're almost there! There are a few things that need to be fixed.

Pull request has been modified.

[sagiegurari]

@siku2 I'll probably need few more days for the docker cleanup comment (a bit busy).
the plan is to add small feature for duckscript to enable to stream the output and get exit code and than i can call the subflows there, than cleanup and if one of the subflows failed, to fail the entire build (but after i cleaned up).
thats a small change but i want to push few more things on the way.

[siku2]

@siku2 I'll probably need few more days for the docker cleanup comment (a bit busy).
the plan is to add small feature for duckscript to enable to stream the output and get exit code and than i can call the subflows there, than cleanup and if one of the subflows failed, to fail the entire build (but after i cleaned up).
thats a small change but i want to push few more things on the way.

Take your time :)

Out of curiosity, wouldn't it make sense to add native support for this to cargo-make?
I believe cleanup tasks are fairly common and relying on Duckscript seems like a bit of a hack (though I of course welcome the idea of making this possible using Duckscript in the first place).

I imagined it something like this:

[tasks.run-tests]
dependencies = ["tests-setup"]
env = { CARGO_MAKE_WORKSPACE_SKIP_MEMBERS = ["examples/*", "yewtil/examples/*"] }
run_task = { name = [ "run-tests-flow", "run-tests-stdweb" ], fork = true }
cleanup_task = "tests-cleanup"

Where the task given to cleanup_task always runs at the end, even if the task itself failed.

[sagiegurari]

i agree it would be better, but unfortunately that would require big design changes.
let me think of something i might be able to do...

[sagiegurari]

@siku2 the cleanup task is now implemented.
i didn't delete the old shell files or updated the docs, so its running side by side until you feel its ok to do the switch.

[siku2]

i didn't delete the old shell files or updated the docs, so its running side by side until you feel its ok to do the switch.

That's alright, I'll take care of that. My plan is to replace the old scripts right away but then there's still some work to be done before we can use it for CI too.
One thing I would like to point out is that the cleanup_task doesn't run if the task is cancelled for instance with SIGINT (CTRL+C). That doesn't hold back this PR but I think cleanup should run in that case too.

There is one last issue that I caused by using a random port for the httpbin server: cargo doesn't detect changes to the content of env! macro variables so the tests won't use the correct URL if nothing else caused a rebuild.
The only solution I can come up with on the spot is to use include_str! (which does detect changes AFAIK) and write the HTTPBIN_URL to a file instead. This solution seems a bit convoluted to me so I want to look into other solutions first.

Other than that I think this is good to go!

[sagiegurari]

before we can use it for CI too.

Feel free to ping me if you need help with that, or if we need new features in cargo-make

cargo doesn't detect changes to the content of env! macro variables

I think the best way forward is to change that to env::var call and not env! macro. so you pick up the right value during execution, not during compilation.

thanks a lot for all the reviews and comments 😃
more than happy to help out more if needed as this project got some interesting challenges.

[siku2]

I think the best way forward is to change that to env::var call and not env! macro. so you pick up the right value during execution, not during compilation.

I should've mentioned that this sadly isn't possible. The tests run in the browser so we can't access the environment variables at runtime.

[sagiegurari]

The tests run in the browser so we can't access the environment variables at runtime.

woops. ya, challenging. so... instead of random, maybe we should use a fixed port value always.
and allow users to change that via env var.
so if env var exists, use that, if not, use default value 5555 or something.

[siku2]

I suppose that's the best option for now but it hurts to see the beautiful port detection logic removed 😄.
The core issue will still be there but at least it shouldn't appear as often.

Pull request has been modified.

[sagiegurari]

@siku2 so based on our talk i did 2 more changes

• docker port is now defaulted to 7117 unless YEW_HTTPBIN_PORT is defined and that will be used instead.
• not related to our talks but i think its important... i am skipping the whole thing if i can't find docker installed. if i do not do that, people without docker will fail to run it.

[siku2]

Alright, it looks like we're pretty much good to go then.

[siku2]

@sagiegurari I did some final testing and I ran into a problem. It seems like "tests-cleanup" already runs before "run-tests-stdweb" does, so when the fetch service tests run for yew-stdweb there's no longer an httpbin server around.
I don't recall encountering this issue before but I also don't see how any of the changes could've caused it so I probably just missed it.

The output looks like this:

... other tests ...

[cargo-make][1] INFO - Build Done in 58 seconds.
[cargo-make] INFO - Execute Command: "docker" "rm" "--force" "<snip>"

[cargo-make] INFO - Execute Command: "cargo" "make" <snip> "run-tests-stdweb"
[cargo-make][1] INFO - Task: run-tests-stdweb

[sagiegurari]

I'll check that. its a new thing (cleanup_task inside run_task). maybe there is a bug there...

[sagiegurari]

Found the issue, publishing an update in few minutes...

[sagiegurari]

should be fine now. thanks for testing :)

Pull request has been modified.

[siku2]

This time I'm confident everything is working.

Thanks so much for all the work you put into this!