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

Resubmission of templating PR (#1747) #3004

Merged
merged 3 commits into from Feb 2, 2017

Conversation

Projects
None yet
9 participants
@ehiggs
Copy link
Contributor

ehiggs commented Aug 17, 2016

This is a manual rebase of the older #1747 PR which was basically unrebasable due to the time it's been dormant.. This implements templating for Cargo and is basically the work of Greg Chapple (@gchp).

I'd love for this feature to move forward since it's really tedious to create all the directories (e.g. src/bin) and copy paste docopt examples which I then edit. Then implement the main program to delegate to the subcommands in src/bin, etc.

@rust-highfive

This comment has been minimized.

Copy link

rust-highfive commented Aug 17, 2016

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@killercup

This comment has been minimized.

Copy link
Member

killercup commented Aug 17, 2016

Nice! I would really love to have this. Copying the same README.md, LICENSE, travis, gitignore, editorconfig, etc. files into every repo can't be the way it's supposed to go.

@gchp

This comment has been minimized.

Copy link
Contributor

gchp commented Aug 17, 2016

Thanks for re-submitting this. I never got the time to come back to it unfortunately, but I think its very useful and would love to see it finished off and merged. Thanks for taking it on @ehiggs!

@gchp

This comment has been minimized.

Copy link
Contributor

gchp commented Aug 17, 2016

The biggest thing not addressed by this from the earlier PR was the use of Handlebars instead of Mustache. At the time the existing Handlebars implementations were a little unstable, but I imagine it's better now so should be easier to work in.

@ehiggs

This comment has been minimized.

Copy link
Contributor Author

ehiggs commented Aug 19, 2016

@gchp agreed. So the work is as follows:

  • Migrate to handlebars.
  • Remove caching mechanism. If users want a local version, they will have to manage the download and copying into ~/.cargo/template manually.
  • Tests
  • Documentation

Future work: cargo template suite of subcommands for managing templates.

@ehiggs ehiggs force-pushed the ehiggs:master branch 2 times, most recently from b713d4e to 0407970 Aug 21, 2016

Cargo.lock Outdated
"openssl-sys-extras 0.7.14 (registry+https://github.com/rust-lang/crates.io-index)",
]

[[package]]
name = "openssl-sys"
version = "0.7.14"
version = "0.7.17"

This comment has been minimized.

@alexcrichton

alexcrichton Aug 22, 2016

Member

It looks like this may have run cargo update, but could this commit just add the dependencies it needs to?

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Aug 22, 2016

Thanks for the revival @ehiggs! Excited to see progress on this

};
let template_dir = templates_dir.join(repo_name);

match Repository::clone(template, &*template_dir) {

This comment has been minimized.

@alexcrichton

alexcrichton Aug 22, 2016

Member

Cargo typically issues a status message for operations that may take a long time, so perhaps this could indicate that a repository is being cloned?

I think this may also wish to use the helpers in git/utils.rs to ensure this handles pieces such as authentication, retries, etc.

template.starts_with("git@") => {
let path = PathBuf::from(template);

let repo_name = match path.components().last().unwrap() {

This comment has been minimized.

@alexcrichton

alexcrichton Aug 22, 2016

Member

This unwrap() should be chain_error w/ a propagation of the error

}

/// Create a new "bin" project
fn create_bin_template(path: &PathBuf) -> CargoResult<()> {

This comment has been minimized.

@alexcrichton

alexcrichton Aug 22, 2016

Member

It seems like these should perhaps be "built in" rather than literally created on the filesystem?

@@ -408,6 +411,65 @@ will not fail your overall build. Please see the [Travis CI Rust
documentation](https://docs.travis-ci.com/user/languages/rust/) for more
information.

# Templates

This comment has been minimized.

@alexcrichton

alexcrichton Aug 22, 2016

Member

This documentation doesn't seem to mention --template name along with ~/.cargo/templates, but maybe that won't be used that much in practice?

This comment has been minimized.

@ehiggs

ehiggs Sep 3, 2016

Author Contributor

It says:

# use the mytemplate template which is located in .cargo/templates/mytemplate   
$ cargo new myproj --template mytemplate   

@ehiggs ehiggs force-pushed the ehiggs:master branch from 8050539 to c39601d Aug 30, 2016

@ehiggs

This comment has been minimized.

Copy link
Contributor Author

ehiggs commented Aug 30, 2016

I've done some work on this and rebased. I changed the way remote repos work by cloning them into a tempdir and using them as a template. The tempdir is then deleted when it goes out of scope. I do something similar with the default bin/lib templates. These are dumped to a tempdir and then used as a template.

I tried to use cargo::sources::git::utils::GitRemote and friends but when I try to clone the remote repository using checkout it seems to only copy the .git directory. I guess this is used to finely figure out which revisions are available or maybe I misunderstood how it's supposed to work since I thought there might be a nice single function to call to get all the work done.

In trying to get the git::utils working, I noticed that it only supports remote git repositories with Urls. This means remote repositories defined using ssh strings (e.g. git@github.com:ehiggs/rust-cmdline-skeleton.git) won't work.

I also made a skeleton repository to test this with. It used docopt and has a LICENSE file and everything to get started writing command line programs that use subcommands. It's basically @BurntSushi's xsv stripped down to... a skeleton 💀 (I keep calling it a skeleton since that's what it's called in adduser.

@ehiggs

This comment has been minimized.

Copy link
Contributor Author

ehiggs commented Sep 7, 2016

I've added some commits that address docs and fix the tests.

@ehiggs

This comment has been minimized.

Copy link
Contributor Author

ehiggs commented Sep 7, 2016

Travis failed due to a network issue. Appveyor failed on "override_cargo_home" but I don't have access to a Windows box to try and reproduce it.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Sep 12, 2016

Thanks for the update @ehiggs! Some thoughts of mine:

  • The management of git templates still feels like we need to tweak the UI here. For example once it's cloned the first time it'll never get updated until it's removed? I'd expect that each time you create a project it'd update the git repo currently.
  • Storing custom templates in ~/.cargo/templates seems like it may be brittle wrt namespace collisions? It's also kinda weird to store user data in a dotfile location. I'd personally prefer if ~/.cargo/templates was entirely Cargo-controlled, for now. (but paths could be specified as template directories still)
  • Could a test be added for a git repository as well?
  • We may want to explore ~/.cargo/config configuration for default templates, I know I'd probably use it!

Also cc @rust-lang/tools, would be good to start hearing thoughts about this as well!

@ehiggs

This comment has been minimized.

Copy link
Contributor Author

ehiggs commented Sep 14, 2016

For example once it's cloned the first time it'll never get updated until it's removed?

There seems to be some confusion here. @gchp's original work cloned the remote repository into ~/.cargo/templates/<repo-name>. I removed that and merely clone the repo into a tempdir which is thrown away when the resulting Cargo project has been generated. So no local updating is required. Any work like that would be done in cargo template clone and/or cargo template pull, type commands which aren't in this PR.

So I guess the resulting work should be:

  • git tests using test/git.rs as an example.
  • Options to set the template dir in ~/.cargo/config
  • Options to set the default template in ~/.cargo/config

For the default template, I would keep --lib and --bin use the explicit defaults; but using no flag would use the default template instead of the --lib one.

@brson

This comment has been minimized.

Copy link
Contributor

brson commented Sep 16, 2016

I like the feature and from reading the docs this seems pretty reasonable.

I've always envisioned project templates being part of the platform, so I would expect us to eventually distribute a bunch of them by default, and for IDEs to have options to instantiate them. This implementation doesn't provide for that yet since it only reads from .cargo and git repos. That's fine since we can improve the feature in the future, but there is one thing here I'm worried could prevent an important enhancement, and that's the way it gets templates from git repos.

As a step toward including templates in the Rust distribution by default I might expect and encourage somebody to maintain a project collecting templates - a single repo that contains nothing but templates. Thus people would naturally want to instantiate templates out of a subdirectory of a git repo, not from the top-level of the repo. This implementation seems to support one template per repo.

@ehiggs

This comment has been minimized.

Copy link
Contributor Author

ehiggs commented Sep 18, 2016

there is one thing here I'm worried could prevent an important enhancement, and that's the way it gets templates from git repos.

In writing the example repository I thought that it might be a pain the backside to maintain several repositories for each skeleton. I think the options would be:

  • Change the work so it uses a git repo and an optional second argument for a subdirectory. This is straightforward to implement.
  • Change the work to be single files and the template just works as a url to the file. This could be a .zip, a .tgz, or maybe even a .patch file which is applied to create the new files and then templated. I would not do something like a Dockerfile or spec since they runs arbitrary scripts and are expected to come from trusted/signed sources. This is more involved but it means that anyone who wants to serve up repos can just toss up a web page with a thin veneer over an object store and serve the templates.
@ehiggs

This comment has been minimized.

Copy link
Contributor Author

ehiggs commented Sep 19, 2016

After discussion with @brson on irc, we think this will work:

New lib: $ cargo new foo --lib
New bin: $ cargo new foo --bin

New local template:

$ grep template ~/.cargo/config
template-dir=~/myrusttemplates
default-template=mytemplate
$ cargo new foo --template mytemplate
$ # OR...
$ cargo new foo # no longer assumes `--lib`; uses `default-template setting`.

New template based on git repo:

$ cargo new foo --template-repo=https://gitlab.com/ehiggs/awesome-template

New template based on git repo where the repo is a collection of templates:

$ cargo new foo --template-repo=https://gitlab.com/ehiggs/awesome-templates --template=best-template

Probably new errors:

$ cargo new foo --template=lib # This will run a template called lib; not the builtin `--lib` version.
$ cargo new foo --template=bin # This will run a template called bin; not the builtin `--bin` version.
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Sep 28, 2016

☔️ The latest upstream changes (presumably #3057) made this pull request unmergeable. Please resolve the merge conflicts.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Sep 30, 2016

@ehiggs oh thanks for the previous clarifications! The strategy you and @brson came up with sounds reasonable to me, but I wonder if we could perhaps forgo the ~/.cargo configuration for now? Or is that required for particular portions?

If you want to rebase I can take another look as well.

@ehiggs

This comment has been minimized.

Copy link
Contributor Author

ehiggs commented Oct 7, 2016

I rebased and removed use of the config. So now there is --template and --template-repo.

The following is supported:

New default library: cargo new --lib foo

New default binary: cargo new --bin foo

New templated project based on remote repo:
cargo new --template-repo https://rustrepos.org/mytemplate foo

New templated project selecting a single template from a bunch in the repository:
cargo new --template-repo https://rustrepos.org/many-repos --template mytemplate foo

Local repo (untested ⚠️ ): cargo new --template-repo file://path/to/template foo

Local directory: cargo new --template-repo /path/to/template foo

Error (template with no associated repo): cargo new --template mytemplate foo

@ehiggs ehiggs closed this Oct 7, 2016

@ehiggs ehiggs force-pushed the ehiggs:master branch from 34881dc to 6534bdd Oct 7, 2016

@ehiggs ehiggs reopened this Oct 7, 2016


let template_dir = match (opts.template_repo, opts.template) {
// given template is a remote git repository & needs to be cloned
// This will be cloned to .cargo/templates/<repo_name> where <repo_name>

This comment has been minimized.

@TheNeikos

TheNeikos Oct 10, 2016

Contributor

This doesn't seem to be true anymore, I think that's a left over from previous iterations.

ehiggs added some commits Aug 16, 2016

Cargo templating for `new` and `init`
PR #3004 This is a resubmission of the PR #1747 (from scratch) which adds
support for templating in Cargo. The templates are implemented using the
handlebars crate (where the original PR used mustache).

Examples:
cargo new --template https://url/to/template somedir foo
cargo new --template https://url/to/templates --template-subdir somedir foo
cargo new --template ../path/to/template somedir foo

@ehiggs ehiggs force-pushed the ehiggs:master branch from 501ff6c to 265452d Jan 31, 2017

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jan 31, 2017

Beta was branched let's merge!

@bors: r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 31, 2017

📌 Commit 265452d has been approved by alexcrichton

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 1, 2017

⌛️ Testing commit 265452d with merge 5ef54e0...

bors added a commit that referenced this pull request Feb 1, 2017

Auto merge of #3004 - ehiggs:master, r=alexcrichton
Resubmission of templating PR (#1747)

This is a manual rebase of the older #1747 PR which was basically unrebasable due to the time it's been dormant.. This implements templating for Cargo and is basically the work of Greg Chapple (@gchp).

I'd love for this feature to move forward since it's really tedious to create all the directories (e.g. `src/bin`) and copy paste docopt examples which I then edit. Then implement the `main` program to delegate to the subcommands in `src/bin`, etc.
@ehiggs

This comment has been minimized.

Copy link
Contributor Author

ehiggs commented Feb 1, 2017

Travis has had problems with the OS X queue. Hence the

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 1, 2017

💔 Test failed - status-travis

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Feb 1, 2017

@bors: retry

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 1, 2017

⌛️ Testing commit 265452d with merge f7fb965...

bors added a commit that referenced this pull request Feb 1, 2017

Auto merge of #3004 - ehiggs:master, r=alexcrichton
Resubmission of templating PR (#1747)

This is a manual rebase of the older #1747 PR which was basically unrebasable due to the time it's been dormant.. This implements templating for Cargo and is basically the work of Greg Chapple (@gchp).

I'd love for this feature to move forward since it's really tedious to create all the directories (e.g. `src/bin`) and copy paste docopt examples which I then edit. Then implement the `main` program to delegate to the subcommands in `src/bin`, etc.
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 2, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing f7fb965 to master...

@bors bors merged commit 265452d into rust-lang:master Feb 2, 2017

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Feb 2, 2017

🎉

Thanks so much again @ehiggs for your patience!

@ehiggs

This comment has been minimized.

Copy link
Contributor Author

ehiggs commented Feb 2, 2017

Thank you for the patience. Now time to go bash out some templates. 😀
And a big thanks to @gchp for his original work!

@gchp

This comment has been minimized.

Copy link
Contributor

gchp commented Feb 6, 2017

@ehiggs I think it's safe to say you far out-did my original work 😄 great job!

carols10cents added a commit to integer32llc/cargo that referenced this pull request Mar 4, 2017

Restore the generated tests module created by `cargo new`
Appears to have been removed unintentionally in rust-lang#3004.

bors added a commit that referenced this pull request Mar 6, 2017

Auto merge of #3799 - integer32llc:restore-generated-tests-module, r=…
…alexcrichton

Restore the generated tests module created by `cargo new`

Appears to have been removed unintentionally in #3004.

Was just working on the book, ran `cargo new adder` with cargo-0.18.0-nightly (6f1b860 2017-02-11), and got this in `src/lib.rs`:

```rust
#[test]
fn it_works() {
}
```

when I expected to get this:

```rust
#[cfg(test)]
mod tests {
    #[test]
    fn it_works() {
    }
}
```

It looks like this was changed as part of #3004 ([removed](875a8ab#diff-149dd4362a3b0dc13b113762713119dfL477), [added](875a8ab#diff-149dd4362a3b0dc13b113762713119dfR678)), I'm assuming unintentionally?

The regression has not yet hit the beta channel; `cargo-0.17.0-nightly (0bb8047 2017-02-06)` generates the src/lib.rs I expect.
@WiSaGaN

This comment has been minimized.

Copy link
Contributor

WiSaGaN commented Aug 11, 2017

A note that this pull-request has been reverted in #3878 in waiting for a RFC.

@ehiggs

This comment has been minimized.

Copy link
Contributor Author

ehiggs commented Aug 11, 2017

Indeed, ongoing discussion is here. It seems there is no consensus yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.