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

POSIX strict mode #303

Open
RichiH opened this issue Jun 23, 2021 · 11 comments · May be fixed by #304
Open

POSIX strict mode #303

RichiH opened this issue Jun 23, 2021 · 11 comments · May be fixed by #304

Comments

@RichiH
Copy link
Owner

RichiH commented Jun 23, 2021

In light of #302 I think we must tighten vcsh up a lot.

https://gist.github.com/EvgenyOrekhov/5c1418f4710558b5d6717d9e69c6e929 & https://jmmv.dev/2018/03/shell-readability-strict-mode.html largely agree but I don't know off-hand if either form is adopted more widely. I would prefer the more verbose set -o form for readability. @mirabilos what would you suggest?

@alerque
Copy link
Collaborator

alerque commented Jun 23, 2021

I'm far mor familiar with set -e instead of set -o errexit, but haven't seem much of either set -u or set -o nounset in use. At the end of the day I don't cane which we use but completely agree we should be using these.

I seem to recall previous discussion about them, but that might have been as far back as when the goal was still to make vcsh a set of sourceable functions.

RichiH added a commit that referenced this issue Jun 23, 2021
Fixes #303

Signed-off-by: Richard Hartmann <[email protected]>
@alerque alerque linked a pull request Jun 23, 2021 that will close this issue
@mirabilos
Copy link
Contributor

mirabilos commented Jun 23, 2021 via email

@RichiH
Copy link
Owner Author

RichiH commented Jun 23, 2021

Yes, I asked on purpose. If anyone knows the hairy bits of shells from 20 years ago and the intricacies of everything up to now, it's you. :)

The inverse is that -u is likely the shake out ALL the silent bugs in production. If there's an intermediary period in which bugs are reported that is less than ideal, but silent bugs are even worse; and afterwards, it should be more resilient.

-e is similar: once bugs are shaken out, it should hopefully make for smoother sailing.

The question is if 2.0.0 should have them or if we should have a grace period. As a major release sees more and wider scrutiny, I would tend towards keeping it in.

@mirabilos
Copy link
Contributor

mirabilos commented Jun 23, 2021 via email

@alerque
Copy link
Collaborator

alerque commented Jul 8, 2021

My current take on this is that it should not go into the v2 release. I do think it should be added to keep the dev process honest, but I think this should be introduced after we overhaul the test suite. I have an idea there are going to be a couple subtle things this might actually break for some people's scripted usage. I would like to get the test suite working using as close to the current logic as possible, then flip this on and see what happens, then fix anything that broke, then ship it (or not depending on how we want the dev cycle to work after that).

@RichiH How does that sit with you?

@alerque alerque removed this from the v2.0.0 milestone Jul 14, 2021
@RichiH
Copy link
Owner Author

RichiH commented Jul 21, 2021

I slept this over a few times. There's a lot of arguments to make in both directions; the strongest one, to me, is that major version changes signal potential breaking changes and a .0.0 is commonly expected to have bugs that need shaking out.

Phrased differently, following the principle of least [user] surprise I believe 2.0.0 is the best time.

The question then becomes if we should wait for 2.0.0 until tests are overhauled; there does not seem to be an immediate need to cut a new release soon, but I am talking about someone else's time (yours) and I don't know what your time plans are for testing.

@alerque
Copy link
Collaborator

alerque commented Jul 27, 2021

I've now slept on your feedback a couple times to, and so far I have not come round to that way of thinking. Two reasons stand out to me.

  1. Changing something significant like the mode the interpreter runs it is not a small thing and hence runs against my №3 point in wanting to release at all. I know it would be (to many) socially acceptable to break something big in a major-dot-zero-dot-zero release, but I really don't want to do that. I want to change as little as possible so we're sure the result is a drop in replacement, just with different distribution (need to build or switch to standalone releases).

  2. I do not expect to have time to overhaul the test suite to be comprehensive enough to trust with a change like this any time soon. Not never, but I have 4 or 5 projects I manage with major chunks waiting on me that I need to prioritize before sitting down to rebase and overhaul the proposed test suite changes and/or fill in any gabs so we can say the whole things is being regression tested.

I'm not saying I can't be convinced, just that I'm not there yet.

@RichiH
Copy link
Owner Author

RichiH commented Jul 28, 2021

I can see how the mental framing of "mode the interpreter runs" leads to a different risk assessment outcome than "make silent errors visible". I am not saying your framing is wrong, it's very valid. Same as mine in #303 (comment) . Under this lens, the current mode of the interpreter is a potentially serious bug. With those different vantage points, it stands to reason that we come out at different conclusions.

A potential middle ground would be another safe harbour 2.0.0 with an intent to release a 3.0.0 with safer interpreter mode.

And yes, I used the word "safe" in both framings within one sentence on purpose.

@mirabilos
Copy link
Contributor

mirabilos commented Jul 28, 2021 via email

@alerque
Copy link
Collaborator

alerque commented Aug 20, 2021

A potential middle ground would be another safe harbour 2.0.0 with an intent to release a 3.0.0 with safer interpreter mode.

Sure, something like this. I think what we call it (2.1 or 3.0 or 5.9) will depend on circumstances when we get there, how it behaves in testing, whether there are any use cases expected to break, etc. But yes whatever work we do on that along with the test suite can and should be for another release. I am still not even convinced it is the way to go, as @mirabilos notes the interpreter mode isn't just safer, it is different with some trade offs that may or may not actually be "safer". I'm more convinced than ever that evaluating whether that made is better for this project should be done after a robust test suite is working with the current mode so we stand a chance of noticing what changes.

@RichiH
Copy link
Owner Author

RichiH commented Aug 20, 2021

If there are major concerns, cutting a major version number seems safest. Either way, let's not block a 2.0 on this.

I have 14d of PTO coming up, hopefully that means some time to churn through too much email and my vcsh backlog.

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 a pull request may close this issue.

3 participants