numworks / epsilon-sample-app-rust

A sample Rust app for the NumWorks graphing calculator
BSD 3-Clause "New" or "Revised" License
50 stars 11 forks source link

Fix the ci by installing nwlink #18

Open RHL120 opened 1 year ago

Ecco commented 1 year ago

Thanks for this PR! It does fix the build indeed, but I'm not sure this is the right solution.

We would like to avoid installing nwlink, specifically because we want to fix a nwlink version in the repository itself. Indeed, this repository is meant as a template, and we want each app to be able to decide on their own nwlink upgrade path.

Unfortunately, it's a bit annoying here because we need the nwlink command in two different places:

  1. In build.rs to embed the icon
  2. In .cargo/config for the cargo install command

So while we could manually write npx nwlink@x.y.z in both places, that would not be very DRY…

Maybe we should use a package.json file? We managed to avoid having one in the C and C++ sample apps, but we might need one here.

RHL120 commented 1 year ago

Thanks for this PR! It does fix the build indeed, but I'm not sure this is the right solution.

We would like to avoid installing nwlink, specifically because we want to fix a nwlink version in the repository itself. Indeed, this repository is meant as a template, and we want each app to be able to decide on their own nwlink upgrade path.

Unfortunately, it's a bit annoying here because we need the nwlink command in two different places:

  1. In build.rs to embed the icon
  2. In .cargo/config for the cargo install command

So while we could manually write npx nwlink@x.y.z in both places, that would not be very DRY…

Maybe we should use a package.json file? We managed to avoid having one in the C and C++ sample apps, but we might need one here.

Hello, I am not a js developer so I know very little about node and I have very little experience in rust. All I know is from reading the rust book and solving the rustlings so pardon me if my question is stupid but is this a valid solution? In build.rs:

    let output = cmd
        .output()
        .or_else(|_| {
            Command::new("npm")
                .args(&["install", "-g", "nwlink"])
                .output()
                .and_then(|_| cmd.output())
        })
        .expect("You should install npm and nwlink");

I think this should work because as far as I can tell, build runs before install