Ticket #2293 (new enhancement)

Opened 14 months ago

Last modified 3 months ago

Simplify MSYS+buildslave integration

Reported by: LRN Owned by:
Priority: patches-accepted Milestone: 0.8.+
Version: 0.8.6p1 Keywords: windows
Cc: bdbaddog, Callek

Description (last modified by dustin) (diff)

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

W32_POSIX_SHELL.patch Download (3.2 KB) - added by LRN 14 months ago.

Change History

Changed 14 months ago by LRN

comment:1 Changed 13 months 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 13 months 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 13 months 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 13 months 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 13 months 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 13 months ago by LRN (previous) (diff)

comment:6 Changed 5 months ago by tom.prince

  • Priority changed from major to patches-accepted

comment:7 Changed 3 months ago by LRN

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

comment:8 Changed 3 months ago by dustin

  • Description modified (diff)

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

Note: See TracTickets for help on using tickets.