Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support variable substitution with toml lists in the same way "${@}" is supported #202

Closed
ssokolow opened this issue Mar 12, 2019 · 31 comments
Assignees
Milestone

Comments

@ssokolow
Copy link

Features Description
One Makefile-ism that I like to use is variables which contain the common options shared between multiple different invocations of a command, to ensure they remain consistent.

(eg. So I can retrofit a CARGOFLAGS onto Cargo to complement RUSTFLAGS.)

However, cargo-make lacking the shell's loose relationship with list-like behaviour rules out the old standby of a space-delimited string and cargo-make panics if you attempt to put something like options = ["--foo", "--bar"] in the [env] section.

(And the @shell runner seems to be attempting some kind of weird, broken quoting on my experiments in feeding it shell-quoted strings.)

It'd be helpful to have an alternative to crafting tasks like this, which bring in a whole separate language just to split a shell-quoted string for want of list-type variables:

[env]
splittable_arguments = "'%s | %s | %s' foo bar baz"
unsplit_argument = "hello world!"

[tasks.example]
command = "sh"
args = ["-c", "printf ${splittable_arguments} \"${unsplit_argument}\""]

In fact, there's actually a case in the default Makefile.toml which doesn't need this, but which would have benefitted from the mindset that produced it. There are quite a few repetitions of --all-features that would be more useful if replaced with something like "${features_opt}" so it'd be easy to replace with nothing or --features="whatever I want".

(That's actually one of the things contributing to my decision that it's easier to skip_core_tasks. I don't want --all-features and I want users of my template to be able to change the set of enabled features for a single run by invoking with --env.)

Describe the solution you'd like

Support some kind of list variable which can be used the same way ${@} is.

(It'd also be nice to support temporarily overriding them using --env but I haven't had time to think about the pros and cons of various approaches to implementing that yet.)

@sagiegurari
Copy link
Owner

so just to be clear, you are looking to replace 1 arg with multiple args, for example

[tasks.something]
command = "somecmd"
args = ["${OPTIONS}"]

and if OPTIONS is a b c
Then it should be translated to:

[tasks.something]
command = "somecmd"
args = ["a", "b", "c"]

if so, ya it would be very powerful, but also a bit risky and flaky.

risky, because unlike ${@} the user expects it to be a single value, but with expansion something else happens which kinda reminds me of sql injection. which is why command arguments in most runtimes (for example: rust, java, node.js, python) do not automatically expand (i'm actually not sure what runtime does automatically expand command arguments when provided as a list and not a single string).

flaky because cargo-make is not bash and doesn't try to be and putting too much bash like behavior makes it do unexpected things.
${@} is different than this because the user has an intent to allow for expansion and expects the command to behave more dynamically. but with simple env vars, it may change in a way that the user did not expect just because it has some separator character inside.

maybe instead (thinking out loud), apart of the env block which defines env vars, i'll have some vars block which defines vars which only cargo-make knows.
than you would be able to define vars as lists, for example:

[vars]
options = ["a", "b", "c"]

and use those in command arguments (not scripts) as follows:

[tasks.something]
command = "somecmd"
args = ["%{options}"]

Than i'll auto expand it if the options var is a list.
it is very understandable to the user that we have a list here and not a single textual value, which makes the command dynamic.

what do you think?

@ssokolow
Copy link
Author

I have no problem with that.

As long as there's some kind of list variable that I can define in one place and then expand into multiple arguments in several other places (including composite variables), it'll meet my needs.

Though, now that %{...} vs. $(...) has me thinking about it, if you don't think it's too niche, it might also be a good idea to have some way to expand it into a single shell-quoted string, for feeding to commands like help2man which take a shell-quoted string as a single argument.

@sagiegurari
Copy link
Owner

decided to go a different path a bit.
introducing another set of variables would be very complex for users and won't be supported everywhere like env vars.
so instead, i've been rereading what you need and thought about making it simpler.
so in order to make it very clear to users that their env var is being split to multiple command line args, i'm introducing functions.

so functions will have a @@FUNCTION_NAME(ARG1,ARG2,ARG3,...) syntax.
So in order to split an env var to multiple command line args you would do the following:

[env]
MULTIPLE_VALUES="1|2|3|4"

[tasks.split]
command = "echo"
args = ["@@split(MULTIPLE_VALUES,|)"]

in runtime it would look like the following:

[cargo-make] INFO - Running Task: split
[cargo-make] INFO - Execute Command: "echo" "1" "2" "3" "4"
1 2 3 4

so env vars with all their capabilities in support will remain and functions will only come to expand on that to workaround their limitations.

you can already test that in the 0.17.0 development branch.

@sagiegurari sagiegurari added this to the 0.17.0 milestone Mar 16, 2019
@ssokolow
Copy link
Author

With this and #201, you've been busy. :)

I'll try to test both either today or tomorrow.

@ssokolow
Copy link
Author

ssokolow commented Mar 16, 2019

I've started poking at it and I just ran across another problem that I hadn't anticipated.

I'm porting the justfile from rust-cli-boilerplate as a test, since it makes use of this feature, and I hit a brick wall: Practically every cargo subcommand I run in the justfile begins its args with either +$channel or +nightly.

(ie. cargo +nightly fmt or cargo +$channel build. The former to override the normal preference to access unstable options and the latter as a means of easily specifying and overriding per-project channel preferences.)

The README doesn't seem to mention any cargo-make-specific way to select a channel to execute a command on and just sticking that into args causes cargo-make to die with "could not find cargo-+$channel in registry" or "could not find cargo-+nightly in registry".

(And I think you can see why it would be... sub-optimal for me to work around that by making every single task a script to defeat cargo-make's auto-install smarts.)

@sagiegurari
Copy link
Owner

@ssokolow
Copy link
Author

ssokolow commented Mar 16, 2019

Never mind. No clue why, but cargo apparently obeys the RUSTUP_TOOLCHAIN environment variable without mentioning it in the section of the Cargo manual on environment variables.

That said, I did find a legitimate problem. Attempting to use space as a separator ("@@split(_build_flags, )") causes it to die with this:

[cargo-make] ERROR - Split expects a single character separator

Supporting space is important because people will expect to be able to use variables like this, as they do in shell scripts and GNU make:

cmd_opts = "--foo --bar --baz"

@ssokolow
Copy link
Author

ssokolow commented Mar 16, 2019

you mean this

https://github.com/sagiegurari/cargo-make#usage-toochain

Yeah. I thought you called it "channel" because of CARGO_MAKE_RUST_CHANNEL and didn't find anything under that name.

EDIT: OK. I'm using toolchain = "nightly" for a handful of tasks and then putting RUSTUP_TOOLCHAIN = "stable" in the [env] section while I decide whether to split apart its current dual duty of declaring the default toolchain (a la rust-toolchain) and documenting the means to override it for the run of a single make task.

@ssokolow
Copy link
Author

ssokolow commented Mar 16, 2019

To clarify, the reason supporting space as a separator is important:

  1. I provide something like cmd_opts = "--foo" which doesn't inherently demonstrate the correct separator.
  2. Users want to add --bar so they assume that, like in all other build automation that feeds lists of arguments around as strings, space is the separator.
  3. They get a cryptic error from some command that received "--foo --bar" rather than "--foo" "--bar".
  4. It causes them (and me, if they ask for help or report a bug) no end of strife, all because I was unable to follow established convention.

@sagiegurari
Copy link
Owner

That said, I did find a legitimate problem. Attempting to use space as a separator ("@@split(_build_flags, )") causes it to die with this:

Will support it.

@sagiegurari
Copy link
Owner

That said, I did find a legitimate problem. Attempting to use space as a separator ("@@split(_build_flags, )") causes it to die with this:

fixed

@ssokolow
Copy link
Author

ssokolow commented Mar 21, 2019

@@split(_build_flags, ) appears to be working but, while testing it, I noticed two points of concern:

  1. The error message when someone accidentally writes $variable rather than ${variable} gives the impression that variables are disallowed, not that cargo-make supports one variant of the Bourne shell's variable syntax but not the other. Confusing to say the least.

    I'd suggest a rustc-esque "Try this instead" suggestion.

  2. If I write this:

    _build_flags = "--features=\"${features}\" ${build_flags}"

    ...I'll get a does not have these features: '""' error from Cargo. That makes sense in the context of cargo-make, but it leaves me without a means of keeping --features unsplit when it passes through @@split(_build_flags, ).

    In this case, it's just a convenience to avoid repetition when some commands need a more constrained set of flags than others but it is a shortcoming in the design, nonetheless.

    The shell script approach to this solution would be to have either split or a companion to it like shell_split which understands quoting using a crate like shell-words, shellwords, or shlex. (ie. --foo --bar="baz quux" would become ["-foo", "--bar=baz quux"].)

    The problem with the shell script approach is that, if I have multiple passes of substitution, I'm responsible for applying the right number of layers of nested and escaped quotes and, if different uses require different numbers of passes, it might not be possible to reconcile different requirements for how much quoting needs to be nested and escaped without the full suite of shell script featurs, such as writing a function which uses "$@" on its arguments.

    (In hindsight, this is the reason I originally wanted proper array variables and something ${@}-like. I can chain those up all I want and, because, spaces are never overloaded to also mean split points, --features=foo bar would never get split. Shell script's approach is taking a flawed idea and then trying to patch it to health, while array variables just solve the problem from the beginning by only converting spaces into token boundaries when you ask for it, not every time you insert one stream of tokens into the middle of another stream of tokens.)

    Either way, it's not the end of the world, but it is a flaw in the design.

@sagiegurari
Copy link
Owner

  1. I'll add more docs to the https://github.com/sagiegurari/cargo-make/tree/0.17.0#command section

  2. i understand that, but try to work within the design approach instead of having cargo-make support everything that shell does which would never happen and never meant to happen.

as for your use case, try:
_build_flags = "--features="${features}";${build_flags}"
@@split(_build_flags,;)

flaw in the design

Try to filter such remarks.
For me, the design is actually very clear and the guideline is:
what you see is what you get, no magic.
The tasks are very structured with attributes and arrays.
thinking that you can bash script everything is wrong and makes what you would assume is readable and straightforward, actually do some magical bash stuff that later you don't understand what just happened.
Basically what you would really like is writing a bash script, with functions (which is supported) but also on windows (in a way also supported).
And what i would really like is a very structure instructions and lots of automation (such as auto install), ability to layer it (company level stuff -> workspace level stuff -> crate level stuff) and what you hate about cargo-make -> usable instantly after installation, no need to install plugins like in other build tools such as maven/grunt/... and no need to write the same build scripts over and over, in every project you start, as you already get a wealth of tasks including a flow for CI, publish, development, ....

@ssokolow
Copy link
Author

ssokolow commented Mar 21, 2019

as for your use case, try:
_build_flags = "--features="${features}";${build_flags}"
@@split(_build_flags,;)

Good idea. Clearly, the late hour is blinding me to obvious directions to search for solutions.

i understand that, but try to work within the design approach instead of having cargo-make support everything that shell does which would never happen and never meant to happen.

I certainly don't want cargo-make to turn into a full-blown scripting language.

The only reason I wrote all that is that I'd just realized right then and there why my intuition kept calling for an array type, and I was "getting that realization onto paper before I forgot".

Try to filter such remarks.

I meant no malice. It's just an observation.

It's a simple fact that it is an inherent shortcoming/weakness/flaw in any design which readily discards and then re-infers tokenization information from strings... like Bourne shell script.

The tasks are very structured with attributes and arrays.

Agreed. My point was that, in hindsight, I should have advocated more vigorously for more support for arrays.

thinking that you can bash script everything is wrong and makes what you would assume is readable and straightforward, actually do some magical bash stuff that later you don't understand what just happened.
Basically what you would really like is writing a bash script, with functions (which is supported) but also on windows (in a way also supported).

No, I'd just like a nice way, with minimal duplication, to allow users to feed custom arguments to the various cargo invocations when some of them have different requirements for which subsets of the arguments can be used.

Any "I want bash scripting"-isms are just an artifact of my still getting used to cargo-make as I work to port my old justfile to it.

For example, if there were an environment variable Cargo would accept in place of --features, I'd use that instead.

And what i would really like is a very structure instructions and lots of automation (such as auto install), ability to layer it (company level stuff -> workspace level stuff -> crate level stuff)

No argument there.

and what you hate about cargo-make

I have no idea what you mean here. The only thing I "hate" about cargo-make is that, at times, the verbosity involved in describing complex tasks makes it feel a bit like WiX and I worry that, to feel comfortable, I might have to write a pre-processor for generating Makefile.toml, similar to what SASS and LESS do for CSS.

usable instantly after installation, no need to install plugins like in other build tools such as maven/grunt/... and no need to write the same build scripts over and over, in every project you start, as you already get a wealth of tasks including a flow for CI, publish, development, ....

Again, agreed. My only issue with the predefined tasks is that they embody a workflow very different from mine. (And, as a UI/UX guy, I have strong opinions on task naming.)

That's why I want things like extend so I can adjust them rather than reinventing the wheel.

@ssokolow
Copy link
Author

I just realized why @@split(_build_flags,;) didn't occur to me.

Note the escaped quotes In the original _build_flags = "--features=\"${features}\" ${build_flags}".

${features} needs to remain unsplit but ${build_flags} needs to be split and both must use space as their internal separator for the ergonomic reasons I mentioned before.

That is, if features="foo bar" and build_flags="--baz --quux", then the end result I'd be trying to inject into a lot of commands without a ton of boilerplate would be ["--features=foo bar", "--baz", "--quux"].

It's not a deal-breaker, but it does mean I'll be duplicating that bit of boilerplate across practically every task block I define. (Another reason for me to consider whipping up a quick little PyTOML script to act as a preprocessor for that, as well as converting blocks of shellscript-like markup into groups of private tasks, depended on by one public task per block, in order to take advantage of cargo-make's support for auto-installing cargo subcommands.)

@sagiegurari
Copy link
Owner

ok, so workaround is

args = [ "--features=\"${features}\"", "@@split(build_flags, )"]

meaning split both of them to 2 args instead of 1 arg?
not sure how many of those you will have, but with the new extend, you could create a private template task with those defined, and extend it in all the other tasks that require those command args.

@ssokolow
Copy link
Author

The workaround is

[tasks.bloat]
# ...
args = ["bloat", "--features=${features}", "@@split(build_flags, )", "${@}"]

[tasks.build]
# ...
args = ["build", "--features=${features}", "@@split(build_flags, )", "${@}"]

[tasks.check]
# ...
args = ["check", "--features=${features}", "@@split(build_flags, )", "${@}"]

[tasks.doc]
# ...
script = ['''
cargo doc --document-private-items --features=${features} @@split(_build_flags, ) ${@}
cargo deadlinks --dir target/${CARGO_BUILD_TARGET}/doc/${CARGO_MAKE_CRATE_NAME}
''']

# etc. etc. etc.

(NOTE: I know the quoting on that tasks.doc is probably wrong. I'm not in a position to remind myself how script and quoting interact at the moment.)

@sagiegurari
Copy link
Owner

in that case you can have a template task that first arg is some env var.
for example ARG1
than in each other task extend the template task and add env attribute with mapping of ARG1 to actual value.
then you don't need to repeat the args

@ssokolow
Copy link
Author

Good idea.

When you think about it, it's kind of funny that I'm the one that's trying to move away from shell script-esque build automation, yet you're the one who keeps realizing when tricks that feel very shell script-inspired (eg. using a variable for the subcommand name rather than its arguments) are viable solutions.

@sagiegurari
Copy link
Owner

i always thought of how to make tasks as reusable functions. i do something similar with do-on-members.
just add to the template task a condition to validate that the env var was provided

@ssokolow
Copy link
Author

I slept terribly today and my schedule was a mess, so I'm going to have to finish porting and testing my justfile tasks tomorrow instead. Sorry about that.

@sagiegurari
Copy link
Owner

no problem

@ssokolow
Copy link
Author

ssokolow commented Mar 27, 2019

OK. I haven't finished testing yet, but here are the things I've tripped over so far:

  1. Now that I've started to experiment with that "Use extend and specify the cargo subcommand in a variable" trick, It does seem that inheriting the value of private is a little awkward. I'm finding far more cases where I need to reset private to false after using extend than where inheriting it produces the desired behaviour by default.

  2. Another potentially useful thing that I don't see any evidence of is a command-line argument for not deleting the /tmp/$USER/run_script/... files generated by script_runner = "@shell" so users can see the code which syntax errors from the shell refer to.

    (I realized that would be useful because I absent-mindedly copied @@split into a script a few days ago day and then, coming back now, I overlooked the mistake on the assumption that it was intentional and indicative of how @shell worked.)

  3. It seems very counter-intuitive that having dependencies = ["_clean_kcov"] (which is shared with my kcov task) in [tasks.clean] would break [tasks.clean] because the CARGO_TARGET_DIR value from [tasks._clean_kcov] would apply to [tasks.clean] too.

    (The intent is that _clean_kcov is shared between my clean and my kcov tasks and ensures that no stale coverage data from a previous run can get folded into the current report.)

    The only reason I noticed that was because I ran makers clean, thought it seemed to complete too quickly (I don't have an SSD), examined the contents of target, and thought "Wait... wouldn't the justfile version have deleted that folder there?"

  4. I was looking at the coverage-kcov task and noticed that I can't see any indication that you're guarding against stale coverage data causing false positives in more recent runs. Are you expecting users to manually clear out existing coverage information when they want to check whether coverage has gone down?

I'll keep porting and testing and let you know what else I find.

@ssokolow
Copy link
Author

In light of what I discovered for how env behaves when used in dependencies, I'm unsure about the proper way to reliably make the -- conditional so I can share the same cargo command template between commands where you generally want to pass the arguments to the inner command, like cargo fmt ... -- ${@} and commands where passing custom arguments to the cargo subcommand via ${@} shouldn't be ruled out.

At the moment, I just maintain two private tasks that are identical except for the --.

[tasks._simple_cargo_command]
category = "Development"
command = "cargo"
args = ["${cargo_subcommand}", "--features=${features}", "@@split(build_flags, )", "${@}"]
private = true

# Like `_simple_cargo_command` but it includes "--" so you don't need to
# type "makers foo -- -- ...` to feed in flags.
[tasks._simple_cargo_command_with_inner]
category = "Development"
command = "cargo"
args = ["${cargo_subcommand}", "--features=${features}", "@@split(build_flags, )", "--", "${@}"]
private = true

Also, that --features=${features} reminds me. I seem to have tripped over one of your "not yet implemented" messages. (I decided to see if --features=@@remove_empty(features) would work.)

Am I running an outdated build or is that still unimplemented in the release candidate?

Finally, in case you were unaware, rustfmt's handling of report_todo and report_fixme has become annoying:

[tasks.fmt-check]
description = "Alias for `cargo +nightly fmt -- --check [args]` which un-bloats TODO/FIXME warnings"
category = "Development"
script_runner = "@shell"
script = ['''
cargo +nightly fmt -- --check --color always ${@} 2>&1 | egrep -v '[0-9]*[ ]*\|'
''']

Still not done porting and testing.

@ssokolow
Copy link
Author

I don't know if you have a more idiomatic way to collapse away duplication with commands that expect their output to be piped, but I was drawing a blank.

# TODO: Regression test that features and build flags got passed in properly
[tasks.dist-supplemental]
description = "Build the shell completions and a manpage, and put them in `dist/`"
category = "Release Builds"
script_runner = "sh"
script = ['''
        mkdir -p dist
        # Generate completions and store them in dist/
        for shell in bash zsh fish elvish powershell; do
            cargo run --features="$features" $build_flags -- --dump-completions $shell > dist/$CARGO_MAKE_CRATE_NAME.$shell
        done
        # Generate manpage and store it gzipped in dist/
        # (This comes last so the earlier calls to `cargo run` will get the compiler warnings out)
        help2man -N 'cargo +$RUSTUP_TOOLCHAIN run -q --features="$features" $build_flags --' | gzip -9 > dist/$CARGO_MAKE_CRATE_NAME.1.gz
''']

Also, I was reminded of a bug I seem to have discovered in rustup and need to remember to report when I have a moment.

This task, or its justfile equivalent, causes rustup to panic in a seemingly probabilistic manner (which line panics varies) at failed printing to stdout: Broken pipe (os error 32).

# TODO: Check what of this cargo-make will install automatically
[tasks.install-rustup-deps]
description = "Install (don't update) nightly and `channel` toolchains, plus `CARGO_BUILD_TARGET`, clippy, and rustfmt"
category = "Dependencies"
script_runner = "sh"
script = ['''
	# Prevent this from gleefully doing an unwanted "rustup update"
	rustup toolchain list | grep -q "$RUSTUP_TOOLCHAIN" || rustup toolchain install "$RUSTUP_TOOLCHAIN" || true
	rustup toolchain list | grep -q nightly || rustup toolchain install nightly || true
	rustup target list | grep -q "$CARGO_BUILD_TARGET (" || rustup target add "$CARGO_BUILD_TARGET" || true
	rustup component list | grep -q 'clippy-\S* (' || rustup component add clippy || true
	rustup component list --toolchain nightly | grep -q 'rustfmt-\S* (' || rustup component add rustfmt --toolchain nightly || true
        echo "DONE"
''']

That's the end of the hand-testing but I won't consider it truly ready for me to move over from just to cargo-make until I've tweaked my test suite to work with the cargo-make version and gotten a full pass from it.

(Speaking of which, tests for the the various dependency-installation tasks will be interesting. I may need to add Vagrant into my test setup so I can start each test with a VM that's missing the cargo subcommands and then roll it back after the test completes.)

@ssokolow ssokolow mentioned this issue Mar 27, 2019
@sagiegurari
Copy link
Owner

Now that I've started to experiment with that "Use extend and specify the cargo subcommand in a variable" trick, It does seem that inheriting the value of private is a little awkward. I'm finding far more cases where I need to reset private to false after using extend than where inheriting it produces the desired behaviour by default.

Makes sense but its expected from using 'extend'. another behavior would be confusing.
which makes run_task as a viable option sometimes as you can define everything in the template task expect the docs, and invoke it from the public task. we talked about it.

Another potentially useful thing that I don't see any evidence of is a command-line argument for not deleting the /tmp/$USER/run_script/... files generated by script_runner = "@shell" so users can see the code which syntax errors from the shell refer to.

verbose would allow you to see the scripts content before invocation. it prints out a LOT of stuff, but since its once every now and than, its not a big deal.
so you will see the script content and the script line before each invocation.

It seems very counter-intuitive that having dependencies = ["_clean_kcov"] (which is shared with my kcov task) in [tasks.clean] would break [tasks.clean] because the CARGO_TARGET_DIR value from [tasks._clean_kcov] would apply to [tasks.clean] too.

I explained about it a lot in the docs. think about having task A and task B depend on C.
Than you have task D that has both A and B as dependencies.
the execution would be C->A->B->D
that is because both A and B are executed and both are depended on C so C is invoked once.
therefore, it makes no sense to set the env in A and B before calling C.
What I do instead is use run_task.
In general i'm not sure why _clean_kcov doesn't know what to clean and kcov does, but you can even wrap both in a task that sets that env for both of them and than runs them.
similar to conditional flows: https://github.com/sagiegurari/cargo-make#combining-conditions-and-sub-tasks

I was looking at the coverage-kcov task and noticed that I can't see any indication that you're guarding against stale coverage data causing false positives in more recent runs. Are you expecting users to manually clear out existing coverage information when they want to check whether coverage has gone down?

Ya that is because old executables are there. i'm open for suggestions how to clean those beforehand.

Also, that --features=${features} reminds me. I seem to have tripped over one of your "not yet implemented" messages. (I decided to see if --features=@@remove_empty(features) would work.)
Am I running an outdated build or is that still unimplemented in the release candidate?

Can you print that out? I don't have such messages so i'm not 100% sure what you mean.

cargo +nightly fmt -- --check --color always ${@} 2>&1 | egrep -v '[0-9][ ]|'

that would never work on windows unless you have bash available.
any chance to use the format task i provide in the core tasks? you can extend it and set the nightly toolchain and update the command line attributes.
you would get the installation instructions for free.
as for the egrep stuff there... i'm not 100% sure what you are doing there...

rustup component list | grep -q 'clippy-\S* (' || rustup component add clippy || true

you sure you need this one? i do it for you in the clippy task

Generally speaking, you have some really complex script based tasks there.
i'm not sure how you can port all those to windows.
break some stuff to smaller parts, or gating some stuff to only linux/mac via conditions might be needed.

@ssokolow
Copy link
Author

which makes run_task as a viable option sometimes as you can define everything in the template task expect the docs, and invoke it from the public task. we talked about it.

It's clear that I'm misunderstanding something. Every time I tried to use run_task for anything like what you're describing, I got an error message that left me convinced me that it wasn't good for anything beyond aliasing names.

Would you mind sharing an example?

verbose would allow you to see the scripts content before invocation. it prints out a LOT of stuff, but since its once every now and than, its not a big deal.
so you will see the script content and the script line before each invocation.

Huh. I thought I tried verbose and just saw the script's contents before any processing was done.

Still, I'd much prefer something that leaves the temporary file intact so I can easily open it in Vim to see what my Syntastic-integrated copy of shellcheck has to say about it and be absolutely, unambiguously sure that I'm looking at the result after any processing done by cargo-make.

(It'd be even better if cargo make included # env_var_name=value comments at the top for any environment variables set by cargo-make before invoking it.)

In general i'm not sure why _clean_kcov doesn't know what to clean and kcov does, but you can even wrap both in a task that sets that env for both of them and than runs them.

You misunderstand. _clean_kcov knows what to clean and it's overriding clean when I depend on it, which I don't want it to do.

My intuition interpreted dependencies = as being like a function call and, when seen via that mental model, _clean_kcov's variables are polluting clean's scope, which should be higher on the stack.

Here are the problematic tasks.

[tasks._clean_kcov]
private = true
command = "cargo"
env = { "CARGO_TARGET_DIR" = "target/kcov" }
args = ['clean', "-v"]

# BUG: This breaks because _clean_kcov's CARGO_TARGET_DIR gets applied to it
[tasks.clean]
description = "Superset of `cargo clean -v` which deletes other stuff this makefile builds"
category = "Development"
script_runner = "@shell"
script = ['''
cargo clean -v "${@}"
rm -rf target/doc/
rm -rf dist/
''']
dependencies = ["_clean_kcov"]

Ya that is because old executables are there. i'm open for suggestions how to clean those beforehand.

I just woke up, so I'll reply to this once I'm more alert.

Can you print that out? I don't have such messages so i'm not 100% sure what you mean.

Sure... after I've had breakfast.

as for the egrep stuff there... i'm not 100% sure what you are doing there...

Squashing down each report of a TODO or FIXME in the code as close to rustfmt's old single-line format for them of them as possible.

That's basically a literal port of the justfile, so, once I've got feature parity with the cargo-make version, I'll probably use @rust, both for portability and so I can pull in regex and finish the job of forcing each TODO/FIXME report back down to a single line. (It's currently two lines and a separating whitespace when grep is done with it.)

you sure you need this one? i do it for you in the clippy task

Note the comment.

# TODO: Check what of this cargo-make will install automatically

A lot of these are direct ports of the justfile tasks that still need to be reworked to take advantage of cargo-make's features and predefined tasks.

The important goal was reaching feature parity with the justfile first (and the justfile doesn't work on Windows) and only then refactoring it further to make things idiomatic and portable in places where cargo-make allowed me to do it "the wrong way" without refusing to run.

Generally speaking, you have some really complex script based tasks there.
i'm not sure how you can port all those to windows.
break some stuff to smaller parts, or gating some stuff to only linux/mac via conditions might be needed.

Some of them will have to be gated to Linux/Mac because they depend on tools like Valgrind and KCachegrind which don't run on Windows.

Given that you don't have any kind of "lint for staying within what @shell knows how to transform" utility and my only access to post-XP versions of Windows is through appveyor, I'll probably replace all but the most trivial uses of @shell with @rust for any scripts which I expect to work on Windows once I've finished refactoring things to be idiomatic.

@ssokolow
Copy link
Author

Ya that is because old executables are there. i'm open for suggestions how to clean those beforehand.

I just woke up, so I'll reply to this once I'm more alert.

The approach I use is adapted from these two sources and, while I do also set a custom CARGO_TARGET_DIR for kcov, the key parts are these lines:

kcov_path="$CARGO_TARGET_DIR/html"

for file in "$CARGO_TARGET_DIR"/"$CARGO_BUILD_TARGET"/debug/$CARGO_MAKE_CRATE_NAME-*; do
    rm -f "$file"
done

cargo test --no-run || exit $?
rm -rf "$kcov_path"

(With $kcov_path being used to build the output paths that are passed to kcov.)

@ssokolow
Copy link
Author

In fact, here's the whole script in case I've stumbled upon edge cases that need to be handled which you missed:

#!/bin/sh
# Adapted from:
# - http://sunjay.ca/2016/07/25/rust-code-coverage
# - https://users.rust-lang.org/t/tutorial-how-to-collect-test-coverages-for-rust-project/650
#
# As of July 2, 2016, there is no option to make rustdoc generate a runnable
# test executable. That means that documentation tests will not show in your
# coverage data. If you discover a way to run the doctest executable with kcov,
# please open an Issue and we will add that to these instructions.
# -- https://github.com/codecov/example-rust

# Ensure that kcov can see totally unused functions without clobbering regular builds
# Adapted from:
# - http://stackoverflow.com/a/38371687/435253
# - https://gist.github.com/dikaiosune/07177baf5cea76c27783efa55e99da89
export CARGO_TARGET_DIR="target/kcov"
export RUSTFLAGS='-C link-dead-code'
kcov_path="$CARGO_TARGET_DIR/html"

for file in "$CARGO_TARGET_DIR"/"$CARGO_BUILD_TARGET"/debug/$CARGO_MAKE_CRATE_NAME-*; do
    rm -f "$file"
done

if [ "$#" -gt 0 ]; then shift; fi # workaround for "can\'t shift that many" being fatal in dash
cargo test --no-run || exit $?
rm -rf "$kcov_path"

for file in "$CARGO_TARGET_DIR"/"$CARGO_BUILD_TARGET"/debug/$CARGO_MAKE_CRATE_NAME-*; do
        if [ -x "$file" ]; then
                outpath="$kcov_path/$(basename "$file")"
                mkdir -p "$outpath"
                kcov --exclude-pattern=/.cargo,/usr/lib --verify "$outpath" "$file" "$@"
        elif echo "$file" | grep -F -e '-*'; then
                echo "No build files found for coverage!"
                exit 1
        fi
done

(The escaped apostrophe in the comment is just to work around a bug in gVim's TOML highlighter.)

@ssokolow
Copy link
Author

ssokolow commented Mar 28, 2019

Can you print that out? I don't have such messages so i'm not 100% sure what you mean.

Sure... after I've had breakfast.

Bah. Now I can't reproduce it anymore.

With this args line...

args = ["${cargo_subcommand}", "--features=@@remove_empty(features)", "@@split(build_flags, )", "${@}"]

...I just get this more reasonable-to-expect output:

[cargo-make] INFO - Execute Command: "cargo" "build" "--features=@@remove_empty(features)" "--release"
error: Package `boilerplate v0.1.0 (/home/ssokolow/Documents/Critical/src/rust-cli-boilerplate)` does not have these features: `@@remove_empty(features)`

(That said, remove_empty would be a lot more useful if, like ${@}, it supported being used with a prefix/suffix so it could be used in options as well as positional arguments.)

@sagiegurari
Copy link
Owner

implemented in 0.17.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants