rust-lang / cargo

The Rust package manager
https://doc.rust-lang.org/cargo
Apache License 2.0
12.39k stars 2.35k forks source link

cargo new --template needs a documented way to pass files through literally #3860

Closed ssokolow closed 7 years ago

ssokolow commented 7 years ago

When I went to test the behaviour of a tweak to my CLI utility boilerplate and ran just build-release, I got an error about "" being incorrectly indented.

After a little investigation, I realized that cargo new --template's default behaviour is incompatible with just because they use the same syntax for substitution.

Here's how --template mangled the project that, up until now, I've been generating new projects from using cp -r rust-cli-boilerplate "$1"; rm "$1"/.git; {some omitted stuff to sed into Cargo.toml}; git init.

diff -u -ur ../rust-cli-boilerplate/justfile ./justfile
--- ../rust-cli-boilerplate/justfile    2017-03-18 10:21:52.823085192 -0400
+++ ./justfile  2017-03-23 10:16:33.320413082 -0400
@@ -47,21 +47,21 @@
 build-release: miniclean build
        @# Depend on miniclean since stripping UPXd executables is fatal
        @printf "\n--== Stripping, SStripping, and Compressing With UPX ==--\n"
-       {{strip_bin}} {{strip_flags}} "{{zz_target_path}}"
+         ""
        @# Allow sstrip to fail because it can't be installed via "just install-deps"
-       sstrip "{{zz_target_path}}" || true
-       upx {{upx_flags}} "{{zz_target_path}}"
+       sstrip "" || true
+       upx  ""
        @printf "\n--== Final Result ==--\n"
-       @ls -sh "{{zz_target_path}}"
+       @ls -sh ""
        @printf "\n"

-# Alias for `cargo check {{args}}` with the default toolchain
+# Alias for `cargo check ` with the default toolchain
 check +args="":
-       cargo check {{args}}
+       cargo check 

-# Alias for `cargo fmt -- {{args}}`
+# Alias for `cargo fmt -- `
 fmt +args="":
-       cargo fmt -- {{args}}
+       cargo fmt -- 

 # Use `apt-get` to install dependencies `cargo` can't (except `kcov` and `sstrip`)
 install-apt-deps:
@@ -80,8 +80,8 @@
        @# Prevent this from gleefully doing an unwanted "rustup update"
        rustup toolchain list | grep -q stable || rustup toolchain install stable
        rustup toolchain list | grep -q nightly || rustup toolchain install nightly
-       rustup toolchain list | grep -q '{{channel}}' || rustup toolchain install '{{channel}}'
-       rustup target list | grep -q '{{target}} (' || rustup target add '{{target}}'
+       rustup toolchain list | grep -q '' || rustup toolchain install ''
+       rustup target list | grep -q ' (' || rustup target add ''

 # Run `install-apt-deps` and `install-cargo-deps`, list what remains.
 @install-deps: install-apt-deps install-cargo-deps
@@ -95,10 +95,10 @@
 # Run a debug build under callgrind, then open the profile in KCachegrind.
 kcachegrind +args="":
        cargo build
-       rm -rf '{{ callgrind_out_file }}'
-       valgrind --tool=callgrind --callgrind-out-file='{{ callgrind_out_file }}' {{ callgrind_args }} 'target/debug/{{ zz_pkgname }}' '{{ args }}' || true
-       test -e '{{ callgrind_out_file }}'
-       kcachegrind '{{ callgrind_out_file }}'
+       rm -rf ''
+       valgrind --tool=callgrind --callgrind-out-file=''  'target/debug/' '' || true
+       test -e ''
+       kcachegrind ''

 # Generate a statement coverage report in `target/cov/`
 kcov:
@@ -139,11 +139,11 @@

 # Remove the release binary. (Used to avoid `strip`-ing UPX'd files.)
 @miniclean:
-       rm -f "{{zz_target_path}}"
+       rm -f ""

-# Alias for `cargo run -- {{args}}` with the *default* toolchain
+# Alias for `cargo run -- ` with the *default* toolchain
 run +args="":
-       cargo run -- {{args}}
+       cargo run -- 

 # Run all installed static analysis, plus `cargo +stable test`.
 test:
diff -u -ur ../rust-cli-boilerplate/README.md ./README.md
--- ../rust-cli-boilerplate/README.md   2017-03-18 10:20:27.496284234 -0400
+++ ./README.md 2017-03-23 10:16:33.360412488 -0400
@@ -134,12 +134,12 @@
 <tr>
   <td><code>check</code></td>
   <td>args (optional)</td>
-  <td>Alias for <code>cargo check {{args}}</code> with the default toolchain.</code></td>
+  <td>Alias for <code>cargo check </code> with the default toolchain.</code></td>
 </tr>
 <tr>
   <td><code>fmt</code></td>
   <td>args (optional)</td>
-  <td>Alias for <code>cargo fmt -- {{args}}</code></td>
+  <td>Alias for <code>cargo fmt -- </code></td>
 </tr>
 <tr>
   <td><code>install-apt-deps</code></td>
@@ -186,7 +186,7 @@
 <tr>
   <td><code>run</code></td>
   <td>args (optional)</td>
-  <td>Alias for <code>cargo run -- {{args}}</code> with the <em>default</em>
+  <td>Alias for <code>cargo run -- </code> with the <em>default</em>
 toolchain.</td>
 </tr>
 <tr>

Just isn't exactly the most obscure thing and I think you can see how this would be a problem, so I hope you'll agree that there needs to be some kind of solution to pass things like justfile (which use {{ and }} in their own syntax) through literally.

(Imagine if I want to include a base.html.incl with the Twitter Bootstrap boilerplate in a web project which uses a templating engine with this sort of Django/Jinja/Twig-style syntax. cargo new --template would mangle that too.)

Heck, in hindsight, it'd make a lot more sense to make it opt-in. Perhaps a backwards-compatible middle ground could be achieved using some kind of "template config file" which, if present, overrides the current behaviour. That'd allow...

alexcrichton commented 7 years ago

cc @ehiggs @gchp

ehiggs commented 7 years ago

Would it be adequate if there was a {verbatim} tag which meant nothing was touched until {endverbatim}? This appears to be the solution used in Django. Maybe {{cargo-verbatim}} to differentiate between underlying nesting?

ssokolow commented 7 years ago

The Django approach assumes you've already opted into parsing the top level of your nested grammars as Django Templates by calling template() on the path to the file.

Using opt-out is already an anti-pattern for templating (most of the files in a given project template are going to be passed verbatim unless you're using something like Yeoman (where the project template is a JavaScript program which asks the user a bunch of questions, then builds some project boilerplate based on the answers)), so it makes no sense to make every {{ ... }} a bomb waiting for someone to forget to mark it as ignored) and that assumes the file format actually allows {{cargo-verbatim}} to be inserted without horrendous hackery.

(Django Templates are already painful enough at their designed purpose for want of the whitespace-gobbling {{-, -}}, {%-, and -%} that Jinja 2 added to the grammar, simply because of how the HTML whitespace collapsing algorithm interacts with things like constructing comma-separated lists which are too complex for the join filter.)

I see this as a less drastic case of of this hyperbolic hypothetical: "Django mangles my EXE download." "It contains {{ and }} as embedded runs of characters. Use a hex editor to insert {% verbatim %} at the beginning and {% endverbatim %} at the end and you'll receive your original, valid EXE when you download it."

ehiggs commented 7 years ago

You raise a lot of good points. For web stuff, people will want to use templates to create projects where, they themselves, will use templates. Also, as Rust may be used in embedded systems, it's not unthinkable to have a few binary blobs sitting around in a template repository and it makes no sense to parse them as templatable files. I didn't really consider these use cases so I'm happy that you raise them.

Your suggestions could simplify the code since we're just going to copy the template tree and read some (presumably) toml manifest file to see which files need to be run through the template engine.

For bike shed fun: do we use toml? What is the toml file called (template-manifest.toml?)? How do we make templates that can create files that have the manifest toml name?

ssokolow commented 7 years ago

How do we make templates that can create files that have the manifest toml name?

I'd suggest using the "rename list" mechanism I proposed as a way to have different LICENSE and .travis.yml files for the template and the projects generated from it.

I didn't bother to think about naming, elegance, or which should be the key and which should be the value, but here's an illustrative example that parsed correctly under PyTOML in the Python REPL:

[renames]
"LICENSE.gpl3" = "LICENSE"
".travis.proj.yml" = ".travis.yml"
"template-manifest.proj.toml" = "template-manifest.toml"

(That should solve the "template repo and generated repos want to do different things with the same name" problem once and for all and do so without any kind of knock-on side-effects.)

ehiggs commented 7 years ago

@alexcrichton there is a lot of good material here, imo. Do you think we should take a step back from the templates and put it through the RFC process?

alexcrichton commented 7 years ago

@ehiggs yeah that seems like a reasonable course of action to me, this hasn't hit stable yet so we can back it out still. If you feel that's the best option, would you be ok preparing a PR?

ehiggs commented 7 years ago

Sure. I can disable it from the command line or I can rip the whole thing out. Any preference?

alexcrichton commented 7 years ago

@ehiggs oh I had an opinion but I'll look at #3878 instead!

ehiggs commented 7 years ago

Templates have been removed pending an RFC. There is a pre-RFC thread here. I think this can be closed for now.

alexcrichton commented 7 years ago

Thanks @ehiggs!