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

Canonicalize source path #1503

Merged
merged 3 commits into from
May 22, 2023
Merged

Canonicalize source path #1503

merged 3 commits into from
May 22, 2023

Conversation

rylev
Copy link
Collaborator

@rylev rylev commented May 17, 2023

This allows users to have "~/some/path/../to/binary" as their source in their spin.toml.

@rylev rylev requested a review from radu-matei May 17, 2023 13:48
@rylev rylev force-pushed the canonicalize-source-path branch from 48b99e6 to 4c99a91 Compare May 17, 2023 14:31
@rylev rylev requested a review from radu-matei May 17, 2023 14:38
@lann
Copy link
Collaborator

lann commented May 17, 2023

You'll have to rebase on main...sorry!

@rylev rylev force-pushed the canonicalize-source-path branch from 4c99a91 to 85df956 Compare May 18, 2023 10:41
@lann
Copy link
Collaborator

lann commented May 18, 2023

Not sure how I feel about this feature; '~'-expansion is usually just a shell feature and while there are some applications that support it, it isn't really all that common in config files ime. I would rather spell out /home/lann/... so that I'm more likely to catch it when reviewing a diff...

@rylev
Copy link
Collaborator Author

rylev commented May 18, 2023

Perhaps I should explain why I'd like to have this feature. My cargo target directory is always set to .cargo_target directory in my home directory. When I switch back and forth between Linux and macOS I have to constantly adjust the source path to fit the machine I'm on. With this feature, I wouldn't have to adjust since it's always in the same place relative to my home directory.

Perhaps you could explain more about what you're worried about? Do we want to discourage source paths that aren't relative to spin application? We already support absolute paths, so I see this as just an extension of that.

@lann
Copy link
Collaborator

lann commented May 18, 2023

It's partly about consistency. Do we support ~ just in source paths? Should we support ~<user> too? Why just ~-expansion and not e.g. $HOME?

@lann
Copy link
Collaborator

lann commented May 18, 2023

But! Your use case seems reasonable to me and I'm definitely not vetoing this or anything.

@rylev
Copy link
Collaborator Author

rylev commented May 19, 2023

@lann I expanded this PR to also support ENV variables using the shellexpand crate. Unfortunately that crate does not support ~$USER expansion, but I don't think that feature is used super often so perhaps that's fine (at least for now)?

@rylev rylev requested review from lann and radu-matei May 19, 2023 12:18
@lann
Copy link
Collaborator

lann commented May 19, 2023

👍 The only other thing I would consider is whether we want to support ~ in runtime config for consistency here. I can't think of anywhere else it would apply.

@rylev
Copy link
Collaborator Author

rylev commented May 19, 2023

@lann that would need to be reconciled with how we handle sqlite databases as well. Can we merge this first and then handle the runtime config case once the sqlite PR lands? I will make an issue for this.

@rylev
Copy link
Collaborator Author

rylev commented May 19, 2023

I opened #1526 to track that we do this consistently.

rylev added 2 commits May 19, 2023 17:51
Signed-off-by: Ryan Levick <ryan.levick@fermyon.com>
Signed-off-by: Ryan Levick <ryan.levick@fermyon.com>
@rylev rylev force-pushed the canonicalize-source-path branch from 3d78212 to 3725e21 Compare May 19, 2023 15:52
Copy link
Collaborator

@lann lann left a comment

Choose a reason for hiding this comment

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

Sorry, had this review comment queued up but never submitted it...

crates/loader/src/local/mod.rs Outdated Show resolved Hide resolved
Co-authored-by: Lann <lann.martin@fermyon.com>
Signed-off-by: Ryan Levick <ryan.levick@fermyon.com>
@rylev rylev force-pushed the canonicalize-source-path branch from d73aec9 to 55d46c8 Compare May 19, 2023 16:12
@rylev rylev merged commit 05ae0aa into main May 22, 2023
@rylev rylev deleted the canonicalize-source-path branch May 22, 2023 07:33
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.

4 participants