Opened 5 years ago

Last modified 3 years ago

#2293 new enhancement

Simplify MSYS+buildslave integration

Reported by: LRN Owned by:
Priority: patches-accepted Milestone: 0.9.+
Version: 0.8.6p1 Keywords: windows
Cc: bdbaddog, Callek, rutsky.vladimir@…

Description (last modified by dustin)

Right now buildslave runs ALL ShellCommand?s using COMSPEC.

This is bad, because COMSPEC is defined to Windows shell interpreter, not Bash (or any other shell). Moreover, buildslave will add "/c" to the command it gets from COMSPEC, if it isn't there already. So even if user re-defines COMSPEC to call a POSIX-compliant shell (which doesn't really work well - see below), it still gets extra "/c", which prevents it from working correctly.

Re-defining COMSPEC is a bad idea anyway, because certain programs (such as libtool) may internally rely on COMSPEC to run things. They expect COMSPEC to point to Windows shell interpreter, and when it isn't...well, you can imagine what happens.

I propose to add an environment variable that can be set for buildslave, which will point to a POSIX-compliant shell to use (i.e. x:/foo/bar/bin/sh.exe --login). It also places the command that should be run into a script file, and makes the shell run that file instead of passing the command as its argument. The reason is that it's easier to handle quoting that way ("-c" option only takes one argument, so it can be difficult to fit large commands into it), and also changes current directory as needed.

Directory change is needed because of the --login option. Using --login is necessary for shell to source the right initialization files (which, among other things, set up the PATH and several other environment variables), however it has a side-effect of setting current directory to "~".

Attachments (1)

W32_POSIX_SHELL.patch (3.2 KB) - added by LRN 5 years ago.

Download all attachments as: .zip

Change History (11)

Changed 5 years ago by LRN

comment:1 Changed 5 years ago by dustin

  • Keywords MSYS cmd COMSPEC libtool ShellCommand shell removed
  • Milestone changed from undecided to 0.8.+

Implicit in your report is the idea that Buildbot *should* be using a POSIX shell to run commands on Windows. This has not been the case with Buildbot in the past, so it would be a pretty shocking change to make.

Outside of that, there are certainly a *lot* of ways to run commands on Windows, so I'm not sure what's best. Perhaps Buildbot should offer an option to use a POSIX shell, and when that's given, try various tricks to find such a shell (including the environment variable you've used in your patch). That would certainly make it easier for people trying to run the same buildfactory on Windows and POSIX.

comment:2 Changed 5 years ago by LRN

Yes, i admit that a very small number (a few thousands, i think) of projects exist that use buildsystems that are only able to work in POSIX shell, and that this patch was written with these projects in mind.

There's no way to find something on W32, unless you've been pointed to it (i.e. environment variable or a registry key). Very small number of conventions exist on W32, and these are mostly not respected by FSW (i mean, seriously, who would put software into "C:Program Files (x86)"?).

I think it is reasonable to expect that buidlslave maintainer knows what kind of things will be run on the slave, and should set up the environment accordingly (i.e. define the environment variable). So it's a matter of shifting the configuration burden from master (removes the need to call 'sh.exe' explicitly) to slave (it needs to define a variable to point at the shell), while also taking some of it off their shoulders (commandline -> shell script file, with some primitive quoting).

comment:3 Changed 5 years ago by dustin

  • Cc bdbaddog Callek added

We generally try to minimize the slave-side configuration - it spreads knowledge of how the build system is configured too thinly. I think explicit is better than implicit, and that this behavior should be explicitly enabled on the master.

There's been talk about running slave-side commands via BAT files, too, rather than the %COMSPEC% commandline, which would also introduce different semantics.

I wonder if we should add a ShellCommand option like windowsMethod that could be used to enable any of these three (and whatever other madness gets dreamed up)?

I'd like to get some guidance from some of the windows folks - bdbaddog? callek?

comment:4 Changed 5 years ago by bdbaddog

If needed I just use something like..

ShellCommand(['c:/cygwin/bin/bash.exe','--login',.. more arguments.)

If the shell needs to be different per slave, can't you add properties per slave, and then use those in the command you run on the slave?

comment:5 Changed 5 years ago by LRN

Well, if it comes to that, why can't you add a shellType option to ShellCommand?, and use it to specify the shell type (Native, POSIX, Windows, maybe something else too). And then it's up to the slave to comply with it, or tell the master that this shell type is not supported.

It's not that i don't like master-side configurations, it's that calling anything with absolute paths looks SO hacky...

Also, the solution from comment:4 won't fix current directory - MSYS still teleports you to ~/ (unless you also insert ['cd', directoryname] somehow into the command). That, or you'd need to hack MSYS configuration.

Last edited 5 years ago by LRN (previous) (diff)

comment:6 Changed 4 years ago by tom.prince

  • Priority changed from major to patches-accepted

comment:7 Changed 4 years ago by LRN

What is the status of this ticket? Is anything being done, or the whole idea is unacceptable?

comment:8 Changed 4 years ago by dustin

  • Description modified (diff)

Neither - the idea is acceptable, but to my knowledge nothing is being done.

comment:9 Changed 4 years ago by rutsky

  • Cc rutsky.vladimir@… added

comment:10 Changed 3 years ago by dustin

  • Milestone changed from 0.8.+ to 0.9.+

Ticket retargeted after milestone closed

Note: See TracTickets for help on using tickets.