Skip to content

New Cargo build system. #165

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

Merged
merged 32 commits into from
Jul 8, 2017
Merged

New Cargo build system. #165

merged 32 commits into from
Jul 8, 2017

Conversation

ehuss
Copy link

@ehuss ehuss commented Mar 13, 2017

I wanted to share a WIP of a new, enhanced build system for Cargo. I was wondering:

  1. Would you would be willing to merge something like this?
  2. Test it out and let me know what you think (any issues, ideas for improvements, etc.)

The main improvements are:

  • Shows inline phantoms for errors/warnings when building.
  • Properly handles cancelling/stopping a build.
  • Pulls in the user's shell environment to make it less likely to have problems.
  • Supports custom environment variables.
  • Interacts more nicely with the on-save syntax checking.
  • Allows you to configure settings for builds (see docs/build.md).
  • Command to assist configuring custom build variants.
  • On-save syntax checking supports different methods (rustc no-trans, cargo check, clippy)

This also closes a few issues: #182, #170, #162, #154, #146, #109

It should be fully functional in its current state, but I still have a long todo/improvement list. Here's a copy in case you are curious:

  • Support custom build variants (passing in any and all settings).
    • Also include interactive command to configure a new variant.
    • Include "environment", and "path"
    • Any custom arguments.
    • working_dir
  • Set "default" project path/package.
  • Cargo "Automatic"
  • Add "--features" support.
  • cargo check
  • test: Message-order: Next/Prev testing
    Test with a variety of message types (to ensure is_main is good)
    Test with already open files and files that need to be opened.
  • Test without cargo manifest
  • Test without sublime project
  • [ ] Run with multiple executables displays this error:
    error: cargo run requires that a project only have one executable; use the --bin option to specify which one to run
    Should we detect this, and bring a popup to select a target?
    Deferred
  • Clippy: Should it automatically use nightly toolchain?
    • Same with Bench?
  • Make links in clippy output clickable.
  • Deal with duplicate messages generated by clippy (filter duplicate messages in general)
  • Make clippy an option for on-save checking.
  • Should the cargo settings be used for SyntaxCheck somehow?
  • [ ] I think the build-language could be improved (coloring file filenames, numbers, etc.) Deferred
  • [ ] Something similar to Anaconda Show Errors (popup with list of errors) Deferred
  • [ ] Other cargo test options?: --doc --no-run --no-fail-fast Deferred
  • Is term "Target" too confusing? Filter or Kind?
    Internally Cargo uses various terms (CompilerFilter, Kind, Target)
  • Update README
  • Fix called with multiple buffers (views) into same file. (Terminates, but otherwise runs correctly. This is something that really needs to be fixed in Sublime.)
  • Check setting "show_errors_inline"? In case someone doesn't like the phantom style.
  • Possibly consolidate the configuration commands into a single command?
  • Add command to configure environment variables (in particular, make it easy to add RUST_BACKTRACE=1).

One last thing, this replaces the "rustc" build variants. I'm not sure what the use case is for those, and whether or not "cargo script" is a sufficient replacement.

@jasonwilliams
Copy link
Member

I agree about the "rustc" build variants, they're not really needed now we have Cargo Script. So im fine with that

@jasonwilliams jasonwilliams added this to the v2.0.0 milestone Mar 29, 2017
@jasonwilliams
Copy link
Member

Looks like there could be some big, potentially breaking changes in this (especially if rust build is being removed)

So marking as a potential for 2.0.0 for now

@jasonwilliams
Copy link
Member

Hi @ehuss, I'm currently on holiday at the moment but back next week.
I'm happy to pull this down and go through the changes you've made and see if we can integrate it as a 2.0.0 release. Would be worth hearing @dten s thoughts too

@jasonwilliams
Copy link
Member

jasonwilliams commented May 22, 2017

@ehuss im back now, how did you want this reviewed?
I don't know if its still w.i.p and you're not ready for me to look at it yet

@ehuss
Copy link
Author

ehuss commented May 22, 2017

Good question. I don't have anything else left that I am planning to do, so it should be ready to go.

I think primarily testing it out and seeing if you have any obvious problems. I've tested on Windows, Mac, and Linux, but it always helps to try on someone else's setup. See if the usage and documentation make sense and is not too confusing.

I have not yet switched the default on-save syntax check to cargo check. I'm currently running with check on my systems, and it seems to be fine (aside from not handling #[test]).

@jasonwilliams
Copy link
Member

jasonwilliams commented May 25, 2017

Not ignoring this just been pretty busy, and maybe this weekend too.
So will try and check this next week

@ehuss are you on the Rust irc server? May be good to catch you in real time when going through this, no worries if not

@jasonwilliams
Copy link
Member

jasonwilliams commented May 30, 2017

Been taking a look at this today, first off great work, I like the documentation you've added around Cargo build and i like the addition of Automatic.
Having a single build system (instead of Rust build and Cargo build) is much better too.

Everything still seems to work fine from my end and nothing is broken.

When i click "Configure Cargo Build" then "Set Target", it sends me to a build menu, like "Automatic", "Bench", "Check" is it supposed to do this?

When attempting to add an environment variable it opens a new window (with RUST_BACKTRACE: 1 already in it commented out) if i type nothing and try to close the windows sublime text crashes, does this happen for you? (ST3 3126) Windows 10

(['Set Target', '--bin, --lib, --example, etc.'], 'target'), What other options could there be apart from --bin and --lib ? Do we need the --example, etc on the end?

@dten are you able to look at this too?

@ehuss
Copy link
Author

ehuss commented May 30, 2017

The "Set Target" sets the default target per build variant. So if you want to run just 1 test file, you choose "Test > Test: test_name". Then when you run the "Test" variant, it always uses that target. I felt like it didn't make sense to have a global default target since targets are often specific to a particular variant. I think it would be nice if it would only show targets for the currently active build variant, but AFAIK it is not possible to ask Sublime which is currently active.

Yes, you found a bug with the environment variables. I just pushed a fix, thanks!

@jasonwilliams
Copy link
Member

jasonwilliams commented May 31, 2017

@ehuss ok i see
Can i help with any of these items on the todo list?

Is term "Target" too confusing? Filter or Kind?

If Cargo is using Target to identify the same thing we should stick with it

@ehuss
Copy link
Author

ehuss commented May 31, 2017

The remaining unchecked items are mostly nice-to-haves or random thoughts. I'll go through and clear out what's left and add a few more tests. I guess the only issue I was really uncertain about is:

Should Clippy and Bench default to running on nightly? Clippy should work correctly without that if you have your PATH/LD_LIBRARY_PATH set correctly, but I assume most people don't do that (it would be nice if that was fixed in rustup). Also, if clippy is later distributed as a rustup component, it won't be necessary. I have no idea what the future holds for "bench".

@urschrei
Copy link

Just wondering how we feel about merging this?

@ehuss
Copy link
Author

ehuss commented Jun 28, 2017

I'm ready for it to be merged and to address any issues that crop up.

@jasonwilliams
Copy link
Member

Hi guys, I've been a way lately but should have some time next week to look at this, and hopefully we can get a 2.0.0 release going with this in

@jasonwilliams
Copy link
Member

Im hopefully going to give this a final check this weekend, but before i do it would be great if @urschrei or @dten could give some feedback before we merge.

Jase

@jasonwilliams jasonwilliams merged commit 92f8880 into rust-lang:master Jul 8, 2017
@jasonwilliams
Copy link
Member

Looks good to me, merged

@urschrei
Copy link

urschrei commented Jul 9, 2017

Amazing. Thanks @Jayflux and @ehuss for putting all this work in.

urschrei pushed a commit to urschrei/sublime-rust that referenced this pull request Jan 30, 2018
* New Cargo build system.

* Fix wrapping of commands in GitHub flavored markdown.

* Minor fixes to unittests for Rust 1.16.

* Add support for "cargo check".

* Support clippy for on-save checking.

* Fix on-save syntax check running clippy multiple times.

* Add ability to configure cargo build features.

* Support setting environment variables.

This includes a dependency on shellenv to capture the user's environment from their login shell.

* Normalize paths when dealing with settings.

This fixes some problems running tests on Windows.

* Disable on-load message display for tests.

This was causing random breakage (particularly on Windows).

* Fix race condition in tests manifested on linux.

* Add support for custom build variants.

This includes some significant changes to how the current
package is selected.  It should work a little better if you
have multiple packages.

* Add support for a default path/package.

* Add "Automatic" build variant.

* Stop looking for JSON output during Cargo build once compile is finished.

This fixes issues with "cargo run" if your program outputs { at the start of a line.

* Better fix for checking when we should stop looking for JSON output.

The previous fix caused issues with Clippy.

* Remove debug print left behind.

* Minor updates for rust 1.17.

* Change message tests to check the region of the message.

* Fix error highlighting for nested macros.

* Fix clearing of error regions.

* Add rust_phantom_style and rust_region_style config settings.

Fixes rust-lang#182.

* Honor show_errors_inline config setting for Cargo build.

* Fix links in messages that are surrounded by angled brackets.

* Consolidate all Cargo config commands into a single command.

Add commands to configure extra arguments and environment variables.

* Document the new build configure command.

Also document the `default_path` option.

* On-save syntax checking now uses configuration settings from the build
system.

* Finish documenting all settings, and include link to new build docs.

* Fix Cargo environment variable config command when selecting "all build
commands".

* Only print warning about sublime-project when saving setings.

* Fix target detection for cdylib.

Adapted from rust-lang#186, thanks @smbolton.

* Update tests for new messages in rust 1.18.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants