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

testscript: some fixes for Plan 9 #115

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

fhs
Copy link

@fhs fhs commented Nov 18, 2020

Plan 9 environment variables names can't have slash in them.
Removing this is not a big loss considering it's not available in
cmd/go testscripts and it's also not documented.

Update #90

@mvdan
Copy link
Collaborator

mvdan commented Nov 18, 2020

Agh. Maybe we should use a different name? Not being able to use the path separator env var in all platforms is unfortunate.

At the same time, I honestly never test on Plan9.

@mvdan
Copy link
Collaborator

mvdan commented Nov 22, 2020

I meant a different name than /, but still the same name across all OSs. How about slash, which is still short enough? path is too close to $PATH, which already has a different meaning.

@fhs
Copy link
Author

fhs commented Nov 22, 2020

Sorry, maybe I should've create a separate PR for the $path change, which is fixing a different issue on plan9. It's just a port of an upstream CL.

As for the / issue, I don't have any preference on a name. Any name without / (including names containing multi-byte unicode characers) will work on plan9. There are many options if you want something short: $|, $\, , , etc.

@mvdan
Copy link
Collaborator

mvdan commented Nov 22, 2020

Ah, gotcha.

I don't think any of the other single-character env names are particularly intuitive, to be honest. I'd go with slash, but feel free to wait for @rogpeppe in case he disagrees.

@fhs fhs changed the title testdata: remove ${/} on Plan 9 testdata: some fixes for Plan 9 Nov 22, 2020
@fhs fhs changed the title testdata: some fixes for Plan 9 testscript: some fixes for Plan 9 Nov 22, 2020
We're changing the name because ${/} doesn't work on Plan 9.
Plan 9 environment variables are stored as files in `/env`,
so they can't have slash in them.

Update rogpeppe#90
@mvdan
Copy link
Collaborator

mvdan commented Jun 14, 2022

We dropped this, my apologies.

The change LGTM with one nit: backwards compatibility. We need to keep ${/} working, on platforms other than plan9, otherwise we will likely be breaking users. The new form, ${slash}, would work on all platforms - and those projects wishing to support testing on plan9 can switch to that form.

Also, does plan9 need to expose both $path and $PATH? If not, I would expose just $path, like it's done for $HOME via homeEnvName.

@rogpeppe
Copy link
Owner

tbh I prefer the solution chosen in this CL: https://go-review.googlesource.com/c/go/+/416554
We keep ${/} around but don't export it, so no need for $slash.
Any reason that wouldn't work here?

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.

None yet

3 participants