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

fix: execute script relative to current working directory #323

Merged
merged 4 commits into from
Aug 3, 2024

Conversation

DrunkenToast
Copy link
Contributor

@DrunkenToast DrunkenToast commented Jul 31, 2024

Intends to fix #322

I mostly just quickly wrote something so it would work for my use-case, so I didn't test all languages.


  • Test all languages
  • Take presentation path instead of cwd

src/execute.rs Outdated Show resolved Hide resolved
@DrunkenToast DrunkenToast marked this pull request as ready for review August 1, 2024 18:44
@DrunkenToast
Copy link
Contributor Author

DrunkenToast commented Aug 1, 2024

Just a note for this PR:
This is only for the cwd of the process it runs from.

Bash works perfectly fine and as you would expect, however Javascript for example won't behave exactly as you'd expect.
Require statements will look relative to where the file/module is located, not from the current working directory. e.g. process.cwd() => works, require('package') => breaks, require('./my-module') => breaks.
And ultimately, this can be a thing with other languages/platforms as well, but it's a bit out-of-scope for this project I think.

A workaround with node for this would be something like this require(process.cwd() + '/node_modules/package') or require(process.cwd() + '/my-module'). Which is still way better than what it was before but not pretty.

Personally I am content with this, since I mostly use bash but wanted to put this out there.

@mfontanini
Copy link
Owner

however Javascript for example won't behave exactly as you'd expect

I think this is no worse than it currently is., In fact it is still better because you can do something to get the imports to work without hardcoding the presentation's path, which you couldn't before.

Thanks!

@mfontanini
Copy link
Owner

Wait I think the current directory shouldn't be the default, it should be the base for where the presentation is. This assumes your cwd is the same as where the presentation is but that's not necessarily the case. This base oath is passed around, but not sure if it's accessible from here. We can merge this as is and I can look at it afterwards otherwise (still need the c and c++ changes before merging).

@DrunkenToast
Copy link
Contributor Author

Wait I think the current directory shouldn't be the default, it should be the base for where the presentation is. This assumes your cwd is the same as where the presentation is but that's not necessarily the case. This base oath is passed around, but not sure if it's accessible from here. We can merge this as is and I can look at it afterwards otherwise (still need the c and c++ changes before merging).

Will look into it this evening.

As for the cwd being the presentation path: I was thinking about that too but a lot of applications behave like this so I didn't go through with it. But I think you're right, it would probably be best to take the path where the slides themselves are located.

@DrunkenToast
Copy link
Contributor Author

Wait I think the current directory shouldn't be the default, it should be the base for where the presentation is. This assumes your cwd is the same as where the presentation is but that's not necessarily the case. This base oath is passed around, but not sure if it's accessible from here. We can merge this as is and I can look at it afterwards otherwise (still need the c and c++ changes before merging).

I resolved both of these.

@DrunkenToast DrunkenToast requested a review from mfontanini August 2, 2024 18:05
@mfontanini
Copy link
Owner

Looks like cargo fmt issues again. This actually requires running cargo +nightly fmt because there's a few nightly options enabled in rustfmt.toml, so this may be the issue.

@calebdw
Copy link
Contributor

calebdw commented Aug 3, 2024

@mfontanini, yes---I found that I had to specifically override the repo directory to use nightly (rustup override set nightly) . I'm not that familiar with Rust, but surely there can be a way to specify that nightly is required in the project configuration instead of everyone having to manually override?

@mfontanini
Copy link
Owner

Not sure about that. There's a way to set the project to build on nightly but you don't really want to enforce that, all you need is the cargo fmt to run on nightly. I've personally changed my editor to always use cargo +nightly fmt when formatting because it has no apparent drawbacks and ensures you get the nightly formatting rules whether it's enabled in a project or not.

@mfontanini mfontanini merged commit d221b0a into mfontanini:master Aug 3, 2024
6 checks passed
@DrunkenToast
Copy link
Contributor Author

You can create a toolchain file: https://rust-lang.github.io/rustup/overrides.html#the-toolchain-file
I wanted to create one in the beginning and submit a PR because I couldn't compile the project on stable rust but I was just one version behind.
If this project requires nightly, I highly recommend using a toolchain file.

@DrunkenToast DrunkenToast deleted the fix/code-cwd branch August 3, 2024 21:15
@mfontanini
Copy link
Owner

That's the thing, it doesn't require nightly to build. It needs nightly to be formatted. I've used a toolchain file but to specify it needs to build with it. I don't know if running cargo fmt will automatically use nightly if that's set in the toolchain file.

@DrunkenToast
Copy link
Contributor Author

Yeah sorry, misread it. From quick search online it indeed doesn't seem possible to only enforce fmt. But I do think the toolchain file makes all cargo/rust commands use that toolchain, would need to test it though.
You already have CI in place for it though :)

@calebdw
Copy link
Contributor

calebdw commented Aug 3, 2024

But why would you not just use the same toolchain to build with as well as format with? Seems it would just be easier that way to match?

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.

Running code from CWD
3 participants