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

Add cargo-flux binary #305

Merged
merged 17 commits into from
Jan 10, 2023
Merged

Add cargo-flux binary #305

merged 17 commits into from
Jan 10, 2023

Conversation

cole-k
Copy link
Collaborator

@cole-k cole-k commented Jan 8, 2023

Edit: Usage and installation details are now in flux-docs

Testing locally

Change https://github.com/liquid-rust/flux/pull/305/files#diff-e3670cdab561aaacb93c3521ffc0db64aa666949dcfc0e5e7f0148fd595ef5afR18 to your flux binary path.

cargo install --path flux/bin

Note that this will clobber a pre-existing binary, so if you put your cargo-flux shell script there, you'll need to copy it elsewhere if you don't want it to disappear.

@ranjitjhala
Copy link
Contributor

Nice! Can you update the relevant bit in the docs?

@cole-k
Copy link
Collaborator Author

cole-k commented Jan 9, 2023

Haven't updated the docs yet but you should now be able to just do

cargo install --path flux
cargo install --path flux-bin

I'd love to just be able to do cargo install, but seems that isn't possible with the way things are set up.

@nilehmann any suggestions on how we might be able to reconfigure? Should I add a [package] and containing [[bin]]s to the main Cargo.toml?

I'm also content with leaving it as-is though, it seems like a significant improvement in the install experience.

@cole-k
Copy link
Collaborator Author

cole-k commented Jan 9, 2023

We could also add a binary that generates the vscode plugin json.

{
  "rust-analyzer.checkOnSave.extraEnv": {
    "RUSTC_WRAPPER": "/path/to/flux/target/release/flux",
    "RUSTUP_TOOLCHAIN": "nightly-2022-10-11",
    "LD_LIBRARY_PATH": "/path/to/.rustup/toolchains/nightly-2022-10-11-x86_64-apple-darwin/lib"
  }
}

Although personally I feel like it would make more sense to have this be generated when you run cargo build. Not sure how to do that or where to put it, though.

With some more investment, we could also add a script that would modify the existing plugin json, but I'm not sure whether it's woth it.

@nilehmann
Copy link
Member

nilehmann commented Jan 9, 2023

I think this is good enough. You can do cargo build --release that will put all the binaries in /path/to/flux/target/release and then you can set up your vscode as

{
  "rust-analyzer.checkOnSave.overrideCommand": [
    "/path/to/flux/target/release/cargo-flux",
    "check",
    "--workspace",
    "--message-format=json"
  ]
}

.map(|channel| channel.name().to_string())
}

pub fn get_dyld_fallback_library_path(rust_toolchain: &str) -> Result<PathBuf, i32> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be called something that's not macos specific.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I picked the linux name (ld_library_path) since it seems the most descriptive, but I'm open to suggestions.

flux-bin/src/utils.rs Outdated Show resolved Hide resolved
@cole-k
Copy link
Collaborator Author

cole-k commented Jan 10, 2023

I think this is good enough. You can do cargo build --release that will put all the binaries in /path/to/flux/target/release and then you can set up your vscode as

{
  "rust-analyzer.checkOnSave.overrideCommand": [
    "/path/to/flux/target/release/cargo-flux",
    "check",
    "--workspace",
    "--message-format=json"
  ]
}

If you ran cargo install --path flux/bin && cargo install --path flux would you be able to even write it as below?

{
  "rust-analyzer.checkOnSave.overrideCommand": [
    "cargo-flux",
    "check",
    "--workspace",
    "--message-format=json"
  ]
}

@nilehmann
Copy link
Member

If cargo-flux is in your path and flux is in the same directory then yes you would be able to write it like that.

But what is this cargo install --lib? It doesn't work for me. I can do cargo install --path flux-bin and cargo install --path flux.

@cole-k
Copy link
Collaborator Author

cole-k commented Jan 10, 2023

If cargo-flux is in your path and flux is in the same directory then yes you would be able to write it like that.

But what is this cargo install --lib? It doesn't work for me. I can do cargo install --path flux-bin and cargo install --path flux.

Yeah sorry, I meant cargo install --path. Fixed.

@cole-k
Copy link
Collaborator Author

cole-k commented Jan 10, 2023

I've updated the readme with the new instructions. I tried to follow them myself but it would help if one of you could confirm that it works.

I also added a FLUX_PATH env variable so you can run a different version of flux if desired. I feel like there isn't really a difference between running

cargo install --lib flux

and

cargo build

but maybe if you have the muscle memory for the latter, you will find that env variable helpful.

Copy link
Member

@nilehmann nilehmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cole-k the code looks good. Just left you some small stylistics suggestions.

@ranjitjhala made this page https://liquid-rust.github.io/flux/. We you should add the changes to the docs there. The page can be edited here https://github.com/liquid-rust/flux/tree/main/flux-docs/src. We should also probably remove most of the text from the README and just put links to the doc

flux-bin/src/utils.rs Outdated Show resolved Hide resolved
flux-bin/src/utils.rs Outdated Show resolved Hide resolved
@@ -2,6 +2,13 @@
edition = "2021"
name = "flux"
version = "0.1.0"
default-run = "flux"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏽

@cole-k
Copy link
Collaborator Author

cole-k commented Jan 10, 2023

Moved the readme's contents to flux-docs.

@cole-k cole-k marked this pull request as ready for review January 10, 2023 18:12
@nilehmann
Copy link
Member

@cole-k I think this is ready to go as soon as you resolve the conflict

@cole-k cole-k merged commit 0da21dd into flux-rs:main Jan 10, 2023
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