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

Call yarn as "yarn.cmd" on Windows (fixes #53) #65

Closed
wants to merge 2 commits into from

Conversation

lynn
Copy link

@lynn lynn commented Nov 3, 2022

Also rename the binaries that call yarn so that there are not two clashing binaries named tsync.exe (see #53 (comment)).

@Wulf Wulf mentioned this pull request Nov 11, 2022
Copy link
Owner

@Wulf Wulf left a comment

Choose a reason for hiding this comment

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

@lynn thanks a bundle for this! The latest commit in main now uses tsync as a library instead of expecting a globally-installed tsync binary. How does that affect these changes?

Comment on lines -83 to +84
name = "fullstack"
path = ".cargo/bin/fullstack.rs"
name = "run_yarn_fullstack"
path = ".cargo/bin/run_yarn_fullstack.rs"
Copy link
Owner

Choose a reason for hiding this comment

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

is this rename necessary or were you trying to maintain the naming convention with tsync?
if it's not necessary, let's undo this change.

Comment on lines -87 to +88
name = "tsync"
path = ".cargo/bin/tsync.rs"
name = "run_yarn_tsync"
path = ".cargo/bin/run_yarn_tsync.rs"
Copy link
Owner

Choose a reason for hiding this comment

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

8.1.0 doesn't run tsync via yarn anymore nor does it use the global tsync binary.

Since you can uninstall tsync via cargo uninstall tsync and use cargo tsync normally, do you think this resolves the issue?

Comment on lines +4 to +8
#[cfg(windows)]
pub const YARN_COMMAND: &'static str = "yarn.cmd";

#[cfg(not(windows))]
pub const YARN_COMMAND: &'static str = "yarn";
Copy link
Owner

Choose a reason for hiding this comment

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

🎉

Copy link
Owner

Choose a reason for hiding this comment

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

perfect!~

Comment on lines +5 to +16
#[cfg(windows)]
pub const YARN_COMMAND: &'static str = "yarn.cmd";

#[cfg(not(windows))]
pub const YARN_COMMAND: &'static str = "yarn";

pub fn main() -> Result<(), io::Error> {
let dir = env!("CARGO_MANIFEST_DIR");

println!("Running `yarn tsync` in `$project_dir/frontend/`...");

Command::new("yarn")
Command::new(YARN_COMMAND)
Copy link
Owner

Choose a reason for hiding this comment

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

if you rebase, these changes won't be necessary :)

@Wulf
Copy link
Owner

Wulf commented Feb 11, 2023

@lynn I just want to thank you one more time for this. I applied your command selecting strategy in #128, and when it gets merged in, hopefully windows users will no longer see these issues.

I'm going to close this but feel free to re-open it or the related issue.

@Wulf Wulf closed this Feb 11, 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.

2 participants