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

Add OS-specific shells and commands #12

Closed
ostev opened this issue Jun 30, 2021 · 32 comments
Closed

Add OS-specific shells and commands #12

ostev opened this issue Jun 30, 2021 · 32 comments
Assignees
Labels
enhancement New feature or request

Comments

@ostev
Copy link
Contributor

ostev commented Jun 30, 2021

Currently, Bonnie uses cmd on Windows, with no option to change it to something else — PowerShell, for instance. This presents difficulties when trying to use the various features of PowerShell. For example, (get-command pwsh).Path is valid PowerShell, but cmd says that .Path was unexpected at this time. This is why I propose the following:

  • Add the ability to have customise the shell you're using for a project. I propose a shell field be added at the top-level to bonnie.toml, as follows:

    shell = "string" # Normal syntax
    # Or this:
    shell = {
        default = "optional string"
        linux = "optional string"
        macos = "optional string"
        windows = "optional string"
    }   

    default would be used if one or more platforms was not specified; the default for default would be the current behaviour.

  • Add the ability to have OS-specific commands. This would be useful if you were using equivalent commands on different platforms — which <command> and (get-command <command>).Path for example. I propose that commands should be able to be specified as a record, as well a string. This would look like the following:

    [scripts]
    example = "command" # Normal syntax
    # Or this:
    example = {
        default: "optional string"
        linux = "optional string"
        macos = "optional string"
        windows = "optional string"
    }

    Again, default is used if one or more platforms are not specified. If one or more platforms are not specified and default is not specified, an error would be thrown.

  • Change the default shell on Windows to Windows PowerShell. This would be a breaking change that would bring Bonnie on Windows closer to Bonnie on Linux and macOS — ls is not valid cmd, but it is valid PowerShell. Modern, open-source PowerShell is probably too new to make the default, but Windows PowerShell has been available since 2006.

@arctic-hen7
Copy link
Owner

Haha I was going to open this exact issue this afternoon! I think a top-level shell property is a great idea!

@arctic-hen7
Copy link
Owner

The OS-specific commands proposal may conflict with the subcommand syntax that's in development, so I'll need to think about some way of circumventing it.

@arctic-hen7
Copy link
Owner

One issue I do find is the lack of flexibility with predefining OS strings. Instead, I think it might be better if we had a series of defaults (Windows, MacOS, Linux, maybe BSD), but provided the option of providing a custom Rust target, which gives extreme flexibility over different commands on Musl architecture etc. The only problem would be coming up with a regular expression that can determine a valid target...

@arctic-hen7 arctic-hen7 self-assigned this Jul 1, 2021
@arctic-hen7 arctic-hen7 added the enhancement New feature or request label Jul 1, 2021
@ostev
Copy link
Contributor Author

ostev commented Jul 1, 2021

Perhaps a global per-machine setting would be in order for shell — a ~/.bonnie/settings.toml. I would be interested in implementing this.

@arctic-hen7
Copy link
Owner

Maybe. In the meantime, I have a proposal on OS-specific commands that doesn't conflict with the subcommand syntax.

Default commands are mandatory, but you can specify alternatives for different targets with the .alts key (maybe we'll call it something different but whatever for now):

[scripts]
test = "blah command"
test.alts.windows = "blah command for windows"
test.alts.this-is-an-imaginary-obscure-target = "blah command for obscure system"

What are your thoughts on that syntax? The only drawback I immediately see is having default commands be mandatory.

@arctic-hen7
Copy link
Owner

I will admit, I'm not hugely keen on the idea of any global Bonnie config, I want to project to be as atomic as possible. Particularly, people should be able to author a Bonnie config file that will have reproducible results (within the limits of the commands they write of course) on any system. Setting any kind of global defaults violate that system.

An alternative is to take advantage of the bonnie init command and specify a default template in global settings. That would mean configuration atomicity is kept, and inconvenience is minimised.

@arctic-hen7
Copy link
Owner

Also, for the information of myself, a non-Windows user, what is the equivalent to sh -c "..." that is used to invoke a PowerShell command so I can provide the option? See this line for what I'm talking about.

@ostev
Copy link
Contributor Author

ostev commented Jul 1, 2021

The Windows PowerShell executable is powershell, and the PowerShell executable is pwsh.

I would think that pwsh is probably old enough to require for dev machines. It's been around since 2017, and Windows PowerShell literally prompts you to install it on launch. The issue is if the GitHub CI doesn't have it installed.

@arctic-hen7
Copy link
Owner

arctic-hen7 commented Jul 1, 2021

I would imagine GitHub CI has it installed, but I guess we'll find out!

@ostev
Copy link
Contributor Author

ostev commented Jul 1, 2021

GitHub is a bad example — they're good about this stuff. Corporate, self-hosted enterprise CI environments are another question.

@ostev
Copy link
Contributor Author

ostev commented Jul 1, 2021

I think your version of the syntax makes sense. It's better than the one I proposed, for certain.

@arctic-hen7
Copy link
Owner

Inline tables have to be on one line unfortunately.

@arctic-hen7
Copy link
Owner

Sorry how exactly does the original proposal violate TOML's syntax? We can nest infinitely with traditional tables...

@ostev
Copy link
Contributor Author

ostev commented Jul 1, 2021

I was wrong. Simply wrong — that is why I deleted the comment. 😀 It does not violate the spec in the slightest. The new syntax makes more sense than the one I proposed and does not violate the TOML spec. A VS Code extension I was using just failed to parse it, and I didn't read all of the relevant section of the spec.

@ostev
Copy link
Contributor Author

ostev commented Jul 1, 2021

I'll start working on a PR. 🔨 I'm not entirely sure on how build targets would fit in, though. Could you please elaborate on that? I think I know what you mean, but I'm not entirely sure.

@arctic-hen7
Copy link
Owner

Okay great! I think you should probably wait to start working until I've finalised the command domains and multi-stage commands specifications, because they will create new syntax that is still in flux and may impact your work.

@ostev
Copy link
Contributor Author

ostev commented Jul 1, 2021

Okay 👍

@arctic-hen7
Copy link
Owner

Unfortunately that work is going to be delayed slightly by #13, but I'll try to get a formal syntax specification done by end of day so we have some idea of where exactly we're progressing.

@arctic-hen7
Copy link
Owner

Okay, #13 is fixed so I'll get to work on that spec!

@arctic-hen7
Copy link
Owner

The highly interlinked nature of the three major proposals for Bonnie right now means I think I'm going to set up the foundations of this at least in the code. If you still want to work on it @ostev then I'll try to do the least I can, but I'm going to have to set up the basic parsing.

@ostev
Copy link
Contributor Author

ostev commented Jul 2, 2021

Makes sense.

@ostev
Copy link
Contributor Author

ostev commented Jul 2, 2021

I'm currently working on a Shell abstraction that makes some of the code much cleaner, and reduces some mutability. I'll wait until you're done to continue further work.

@arctic-hen7
Copy link
Owner

arctic-hen7 commented Jul 2, 2021

This is probably getting a bit long for issues, so do you want to continue on Bonnie's Gitter room?

@arctic-hen7
Copy link
Owner

Here's a preliminary formal specification of what I imagine the syntax for all three of these proposals will look like.

@ostev
Copy link
Contributor Author

ostev commented Jul 3, 2021

Makes sense. I'll leave it to you.

@arctic-hen7
Copy link
Owner

Thanks! It'd be really helpful if you could do the global template stuff though, if you want to.

Also, I'm currently rewriting Bonnie from the ground up because refactoring at this point would just create a bird's nest of code, so things should be easier to make sense of and work with soon!

@ostev
Copy link
Contributor Author

ostev commented Jul 3, 2021

As in global settings?

I'll wait until you've finished the refactoring to begin work, since I would have to refactor the code significantly as well — merge conflicts of that sort would be complicated to resolve.

@arctic-hen7
Copy link
Owner

Yeah, as per the idea of a global template in ~/.bonnie/init.toml or the like for setting de facto global settings in an atomised manner.

Oh please do! I am literally rewriting everything at the moment!

arctic-hen7 added a commit that referenced this issue Jul 5, 2021
… and rewrote everything

BREAKING CHANGE: significant error message changes
Closes #4, #11, and #12
@arctic-hen7
Copy link
Owner

This is done except for testing now, though I may still change some things. I'll let you know when I've finalised the release and then you should be able to begin on the global template stuff.

@arctic-hen7
Copy link
Owner

Also @ostev am I correct that powershell -command "{COMMAND}" is how one can run a Powershell command (equivalent for Powershell to cmd /C "{COMMAND}")?

@ostev
Copy link
Contributor Author

ostev commented Jul 6, 2021

Both -command and /C work.

@arctic-hen7
Copy link
Owner

Great, thanks! I'm opening a separate issue for the default to be switched to PowerShell, see #15.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants