-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
azure: drop powershell #5141
azure: drop powershell #5141
Conversation
2d3584b
to
5777ebf
Compare
Oh, jeez. I think that the MSVC builds are failing because they're running inside Git for Windows (aka msys aka mingw) and so it thinks that you want to build them with mingw. And I think that the mingw builds are failing because they don't understand Windows-style paths (eg |
On Mon, Jun 24, 2019 at 11:42:02AM -0700, Edward Thomson wrote:
Oh, jeez. I think that the MSVC builds are failing because
they're running inside Git for Windows (aka msys aka mingw) and
so it thinks that you want to build them _with_ mingw.
And I think that the mingw builds are failing because they
don't understand Windows-style paths (eg `C:\foo`) and you need
to convert them to `/c/foo`. There's an app for this that
ships with cygwin but ughhhh.
Yeah, I expected to have some problems with CMake as I've noticed
in the past that it can be quite weird to use it in Cygwin, too,
when not building with Makefiles. Should've marked this PR as
WIP.
|
You were clear in setting expectations. I just wanted to 👀 to see how bad it was. 😀 I'm heads down on a bunch of things right now but I'm happy to help look at these when I have some time. (ETA for that unknown.) |
d5794f7
to
2e3c875
Compare
Cherry-picked fixes from #5143 as this PR here implicitly enables Werror. |
385d53a
to
e9ab556
Compare
Jeez, this is quickly ballooning in size due to the ENABLE_WERROR stuff. @ethomson, please feel free to either merge this PR in favour of yours or to cherry-pick any fixes. |
d4f8cbe
to
1689a39
Compare
/rebuild |
Okay, @pks-t, I started to rebuild this pull request as build #2268. |
f805f88
to
8620a50
Compare
937f2c8
to
c1b0edf
Compare
@ethomson: finally got this solved. The fix was to not set PATH at all for our jobs, but instead to only adjust PATH for the CMake invocations. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I'm approving but unfortunately I merged the other changes first, so there are conflicts.
c1b0edf
to
9384a7c
Compare
Rebased to fix conflicts |
Instead of having to find the fuzzer executables in our Azure test scripts, provide test targets for each of our fuzzers that will run them with the correct paths.
As different test suites for our CI are mostly defined via CMake, it's hard to run those tests with a summary file path as that'd require us to add another parameter to all unit tests. As we do not want to unconditionally run unit tests with a summary file, we would have to add another CMake build parameter for test execution, which is ugly. Instead, implement a way to provide a summary file path via the environment.
Right now, we have an awful hack in our test CI setup that extracts the test command from CTest's output and then prepends the leak checker. This is dependent on non-machine-parseable output from CMake and also breaks on various ocassions, like for example when we have spaces in the current path or when the path contains backslashes. Both conditions may easily be triggered on Win32 systems, and in fact they do break our Azure Pipelines builds. Remove the awful hack in favour of a new CMake build option "USE_LEAK_CHECKER". If specifying e.g. "-DUSE_LEAK_CHECKER=valgrind", then we will set up all tests to be run under valgrind. Like this, we can again simply execute ctest without needing to rely on evil sourcery.
Since we have migrated to Azure Pipelines, we have deprecated and subsequentally removed all infrastructure for AppVeyor and Travis. Thus it doesn't make a lot of sense to have the split between "ci/" and "azure-pipelines/" directories anymoer, as "azure-pipelines/" is essentially our only CI. Move all CI scripts into the "azure-pipelines/" directory to have everything centrally located and to remove clutter in the top-level directory.
Azure Pipelines supports bash tasks on Windows hosts due to it always having Git for Windows included. To support this, the Git for Window directory is added to the PATH environment to make the bash shell available for execution. Unfortunately, this breaks CMake with the MinGW generator, as it has sanity checks to verify that no bash executable is in the PATH. So we can either remove Git for Windows from the path, but then we're unable to execute bash jobs. Or we can add it to the path, but then we're unable to execute CMake with the MinGW generator. Let's re-model how we set the PATH environment. Instead of setting up PATH for the complete build job, we now set a variable "BUILD_PATH" for the job. This variable is only being used when executing CMake so that it encounters a sanitizied PATH environment without GfW's bash shell.
We're about to phase out our Powershell scripts as Azure Pipelines does in fact support Bash scripts on all platforms. As a preparatory step, let's replace our MinGW setup script with a Bash script.
We currently specify the CMake generator as part of the CMAKE_OPTIONS variable. This is fine in the current setup, but during the conversion to drop PowerShell scripts this will prove problematic for all generators that have spaces in their names due to quoting issues. Convert to use an explicit CMAKE_GENERATOR variable that makes it easier to get quoting right.
Until now, we always had the CC variable defined in the build.sh pipeline. But as we're about to migrate the Windows jobs to Bash, as well, those will not have CC defined and thus we cannot use "$CC" to determine the compiler version. Fix this by only executing "$CC" if the variable was set.
Right now, we maintain semantically equivalent build scripts in both Bash and Powershell to support both Windows and non-Windows hosts. Azure Pipelines supports Bash on Windows, too, via Git for Windows, and as such it's not really required to maintain the Powershell scripts at all. Remove them to reduce our own maintenance burden.
On Win32 build hosts, we do not have an SSH daemon readily available and thus cannot perform the SSH tests. Let's skip the tests to not let Azure Pipelines fail.
On Win32 builds, the PID file created by git-daemon contained in invalid PID that we were not able to kill afterwards. Somehow, it seems like the contained PID was wrapped in braces. Consequentially, kill(1) failed and thus caused the build to error. Fix this by directly grabbing the PID of the spawned git-daemon process.
9384a7c
to
415ee61
Compare
Thanks a lot for your review, @ethomson! :) |
Quick and dirty PR to drop all our Powershell scripts and use Bash across all platforms. I bet it won't succeed yet, but I've been a bit lazy to fire up a Windows host :P