Hi,
Continuing a discussion started over at [1]. Moving this to a new
thread so that other thread can focus on Unix cleanup, and both
threads can get CI coverage...
1. In a few places, it is alleged that both __CYGWIN__ and WIN32
might be defined at the same time. Do you think we should try to get
rid of that possibility? I understand that we have to have a few
tests for __CYGWIN__ here and there, because eg file permissions don't
work quite right and there's not much we can do about that. But it
seems a bit unhelpful if we also have to worry about a more-or-less
POSIX-ish build taking WIN32 paths at uncertain times if we forget to
defend against that, or wonder why some places are not consistent.
A quick recap of the three flavours of Windows platform we have to
handle, as I understand it:
* MSVC: Windowsy toolchain, Windowsy C
* custom perl scripts instead of configure
* msbuild instead of make
* MSVC compiler
* Windows C APIs
* we provide our own emulation of some POSIX C APIs on top
* MSYS: Unixy toolchain, Windowsy C
* configure (portname = "win32")
* make
* GCC compiler
* Windows C APIs
* we provide our own emulation of some POSIX C APIs on top
* Cygwin: Unixy toolchain, Unixy C
* configure (portname = "cygwin")
* make
* GCC compiler
* POSIX C APIs (emulations provided by the Cygwin runtime libraries)
(The configure/make part will be harmonised by the Meson project.)
The macro WIN32 is visibly defined by something in/near msbuild in
MSVC builds: /D WIN32 is right here in the build transcripts (whereas
the compiler defines _WIN32; good compiler). I am not sure how
exactly it is first defined in MSYS builds; I suspect that MSYS gcc
might define it itself, but I don't have access to MSYS to check. As
for Cygwin, the only translation unit where I could find both
__CYGWIN__ and WIN32 defined is dirmod.c, and that results from
including <windows.h> and ultimately <minwindef.h> (even though WIN32
isn't defined yet at that time). I couldn't understand why we do
that, but I probably didn't read enough commit history. The purpose
of dirmod.c on Cygwin today is only to wrap otherwise pure POSIX code
in retry loops to handle those spurious EACCES errors due to NT
sharing violations, so there is no need for that.
Proposal: let's make it a programming rule that we don't allow
definitions from Windows headers to leak into Cygwin translation
units, preferably by never including them, or if we really must, let's
grant specific exemptions in an isolated and well documented way. We
don't seem to need any such exemptions currently. Places where we
currently worry about the contradictory macros could become
conditional #error directives instead.
2. To make it possible to test any of that, you either need a working
Windows+Cygwin setup, or working CI. I'm a salty old Unix hacker so I
opted for the latter, and I also hope this will eventually be useful
to others. Unfortunately I haven't figured out how to get everything
working yet, so some of the check-world tests are failing. Clues most
welcome!
The version I'm posting here is set to run always, so that cfbot will
show it alongside others. But I would imagine that if we got a
committable-quality version of this, it'd probably be opt-in, so you'd
have to say "ci-os-only: cygwin", or "ci-os-only: cygwin, windows" etc
in a commit to your private github account to ask for it (or maybe
we'll come up with a way to tell cfbot we want the full works of CI
checks; the same decision will come up for MSYS, OpenBSD and NetBSD CI
support that my colleague is working on). There are other things to
fix too, including abysmal performance; see commit message.
3. You can't really run PostgreSQL on Cygwin for real, because its
implementation of signals does not have reliable signal masking, so
unsubtle and probably also subtle breakage occurs. That was reported
upstream by Noah years ago, but they aren't working on a fix.
lorikeet shows random failures, and presumably any CI system will do
the same... I even wondered about putting our own magic entry/exit
macros into signal handlers, that would use atomics to implement a
second level of signal masking (?!) but that's an uncommonly large
bandaid for a defective platform... and trying to fix Cygwin itself
is a rabbithole too far for me.
4. When building with Cygwin GCC 11.3 you get a bunch of warnings
that don't show up on other platforms, seemingly indicating that it
interprets -Wimplicit-fallthrough=3 differently. Huh?
[1]
https://www.postgresql.org/message-id/CA%2BhUKGKZ_FjkBnjGADk%2Bpa2g4oKDcG8%3DSE5V23sPTP0EELfyzQ%40mail.gmail.com
From 8f300f5b804d5fd2268709d40e31b52c86d6799c Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.mu...@gmail.com>
Date: Mon, 25 Jul 2022 23:05:10 +1200
Subject: [PATCH 1/2] WIP CI support for Cygwin.
XXX Doesn't get all the way through yet...
XXX Needs some --with-X options
XXX This should use a canned Docker image with all the right packages
installed
XXX We would never want this to run by default in CI, but it'd be nice
to be able to ask for it with ci-os-only! (See commented out line)
XXX configure is soooo slooow, can we cache it?! Compiling is also
insanely slow, but ccache gets it down to a couple of minutes if you're
lucky
XXX I don't know how to put variables like BUILD_JOBS into the scripts
XXX I have no idea if crash dump works, and if this should share
elements with the msys work in commitfest #3575
---
.cirrus.yml | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 51 insertions(+)
diff --git a/.cirrus.yml b/.cirrus.yml
index f23d6cae55..b5238f5f52 100644
--- a/.cirrus.yml
+++ b/.cirrus.yml
@@ -456,6 +456,57 @@ task:
path: "crashlog-*.txt"
type: text/plain
+task:
+ name: Windows - Cygwin
+
+ env:
+ CPUS: 4
+ BUILD_JOBS: 4
+ TEST_JOBS: 8
+
+ #only_if: $CIRRUS_CHANGE_MESSAGE =~ '.*\nci-os-only:[^\n]*cygwin.*'
+
+ windows_container:
+ image: cirrusci/windowsservercore:2019
+ os_version: 2019
+ cpu: $CPUS
+ memory: 4G
+
+ ccache_cache:
+ folder: C:\tools\cygwin\tmp\ccache
+
+ sysinfo_script: |
+ chcp
+ systeminfo
+ powershell -Command get-psdrive -psprovider filesystem
+ set
+
+ setup_additional_packages_script: |
+ choco install -y --no-progress cygwin
+ C:\tools\cygwin\cygwinsetup.exe -q -P cygrunsrv,make,gcc-core,ccache,binutils,libtool,pkg-config,flex,bison,zlib-devel,libssl-devel,libreadline-devel,perl,perl-IPC-Run
+ C:\tools\cygwin\bin\bash.exe --login -c "cygserver-config -y" || EXIT /b 1
+ C:\tools\cygwin\bin\bash.exe --login -c "echo 'kern.ipc.semmni 1024' >> /etc/cygserver.conf" || EXIT /b 1
+ C:\tools\cygwin\bin\bash.exe --login -c "echo 'kern.ipc.semmns 1024' >> /etc/cygserver.conf" || EXIT /b 1
+ C:\tools\cygwin\bin\bash.exe --login -c "net start cygserver" || EXIT /b 1
+
+ configure_script:
+ - C:\tools\cygwin\bin\bash.exe --login -c "cd '%cd%' && CCACHE_DIR=/tmp/ccache ./configure --enable-cassert --enable-debug --enable-tap-tests CC='ccache gcc'" || EXIT /b 1
+
+ build_script:
+ - C:\tools\cygwin\bin\bash.exe --login -c "cd '%cd%' && CCACHE_DIR=/tmp/ccache make -s -j4 world-bin" || EXIT /b 1
+
+ test_world_script:
+ - C:\tools\cygwin\bin\bash.exe --login -c "cd '%cd%' && CCACHE_DIR=/tmp/ccache make -s -j1 check-world -Otarget" || EXIT /b 1
+
+ on_failure:
+ <<: *on_failure
+ crashlog_artifacts:
+ path: "crashlog-*.txt"
+ type: text/plain
+
+ always:
+ upload_caches: ccache
+
task:
name: CompilerWarnings
--
2.35.1
From ea47a8af0332876629b53620788d40bcb7b1e96c Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.mu...@gmail.com>
Date: Tue, 26 Jul 2022 12:56:06 +1200
Subject: [PATCH 2/2] WIP Do not pollute Cygwin namespace with Windows headers.
Establish that __CYGWIN__ and WIN32 should generally not be defined at
the same time. If ever it's necessary to access Windows APIs directly
when building for Cygwin, that should be done in an extremely localized
way, not allowing Windows macros and declarations to leak into other
translation units.
dirmod.c initially looked like a potential case for a localized
exemption, but on closer inspection it doesn't currently have any reason
to include <windows.h> -- so don't.
In passing, remove anachronistic comments about ancient Windows
versions.
---
src/include/pg_config_manual.h | 2 +-
src/include/port.h | 17 +++++++------
src/port/dirmod.c | 46 +++++++++++++++++-----------------
3 files changed, 33 insertions(+), 32 deletions(-)
diff --git a/src/include/pg_config_manual.h b/src/include/pg_config_manual.h
index 5ee2c46267..0d1d5d7c9b 100644
--- a/src/include/pg_config_manual.h
+++ b/src/include/pg_config_manual.h
@@ -148,7 +148,7 @@
* fork()). On other platforms, it's only useful for verifying those
* otherwise Windows-specific code paths.
*/
-#if defined(WIN32) && !defined(__CYGWIN__)
+#if defined(WIN32)
#define EXEC_BACKEND
#endif
diff --git a/src/include/port.h b/src/include/port.h
index d39b04141f..8cf1112a54 100644
--- a/src/include/port.h
+++ b/src/include/port.h
@@ -18,10 +18,12 @@
/*
* Windows has enough specialized port stuff that we push most of it off
* into another file.
- * Note: Some CYGWIN includes might #define WIN32.
*/
-#if defined(WIN32) && !defined(__CYGWIN__)
+#if defined(WIN32)
#include "port/win32_port.h"
+#if defined(__CYGWIN__)
+#error "__CYGWIN__ should not be defined at the same time as WIN32"
+#endif
#endif
/* socket has a different definition on WIN32 */
@@ -150,7 +152,7 @@ extern int pg_disable_aslr(void);
#define EXE ""
#endif
-#if defined(WIN32) && !defined(__CYGWIN__)
+#if defined(WIN32)
#define DEVNULL "nul"
#else
#define DEVNULL "/dev/null"
@@ -276,12 +278,11 @@ extern int pgunlink(const char *path);
* Win32 also doesn't have symlinks, but we can emulate them with
* junction points on newer Win32 versions.
*
- * Cygwin has its own symlinks which work on Win95/98/ME where
- * junction points don't, so use those instead. We have no way of
- * knowing what type of system Cygwin binaries will be run on.
- * Note: Some CYGWIN includes might #define WIN32.
+ * Cygwin has its own symlinks that work where junction points don't, so use
+ * those instead. We have no way of knowing what type of system Cygwin
+ * binaries will be run on.
*/
-#if defined(WIN32) && !defined(__CYGWIN__)
+#if defined(WIN32)
extern int pgsymlink(const char *oldpath, const char *newpath);
extern int pgreadlink(const char *path, char *buf, size_t size);
extern bool pgwin32_is_junction(const char *path);
diff --git a/src/port/dirmod.c b/src/port/dirmod.c
index 7ce042e75d..1bfbead098 100644
--- a/src/port/dirmod.c
+++ b/src/port/dirmod.c
@@ -6,8 +6,10 @@
* Portions Copyright (c) 1996-2022, PostgreSQL Global Development Group
* Portions Copyright (c) 1994, Regents of the University of California
*
- * This includes replacement versions of functions that work on
- * Win32 (NT4 and newer).
+ * This includes replacement versions of functions that work on Windows.
+ * For Cygwin, the purpose of these replacements is to provide retry loops
+ * around POSIX functions. For native Windows, we also redirect to
+ * native Windows APIs.
*
* IDENTIFICATION
* src/port/dirmod.c
@@ -21,25 +23,24 @@
#include "postgres_fe.h"
#endif
+#if defined(WIN32) && defined(__CYGWIN__)
+#error "WIN32 should not be defined at the same time as __CYGWIN__"
+#endif
+
+#if !defined(WIN32) && !defined(__CYGWIN__)
+#error "one of WIN32 or __CYGWIN__ is expected"
+#endif
+
/* Don't modify declarations in system headers */
-#if defined(WIN32) || defined(__CYGWIN__)
#undef rename
#undef unlink
-#endif
#include <unistd.h>
#include <sys/stat.h>
-#if defined(WIN32) || defined(__CYGWIN__)
-#ifndef __CYGWIN__
+#if defined(WIN32)
#include <winioctl.h>
-#else
-#include <windows.h>
-#include <w32api/winioctl.h>
#endif
-#endif
-
-#if defined(WIN32) || defined(__CYGWIN__)
/*
* pgrename
@@ -56,24 +57,24 @@ pgrename(const char *from, const char *to)
* someone else to close the file, as the caller might be holding locks
* and blocking other backends.
*/
-#if defined(WIN32) && !defined(__CYGWIN__)
+#if defined(WIN32)
while (!MoveFileEx(from, to, MOVEFILE_REPLACE_EXISTING))
#else
while (rename(from, to) < 0)
#endif
{
-#if defined(WIN32) && !defined(__CYGWIN__)
+#if defined(WIN32)
DWORD err = GetLastError();
_dosmaperr(err);
/*
- * Modern NT-based Windows versions return ERROR_SHARING_VIOLATION if
- * another process has the file open without FILE_SHARE_DELETE.
- * ERROR_LOCK_VIOLATION has also been seen with some anti-virus
- * software. This used to check for just ERROR_ACCESS_DENIED, so
- * presumably you can get that too with some OS versions. We don't
- * expect real permission errors where we currently use rename().
+ * Windows returns ERROR_SHARING_VIOLATION if another process has the
+ * file open without FILE_SHARE_DELETE. ERROR_LOCK_VIOLATION has also
+ * been seen with some anti-virus software. This used to check for
+ * just ERROR_ACCESS_DENIED, so presumably you can get that too with
+ * some OS versions. We don't expect real permission errors where we
+ * currently use rename().
*/
if (err != ERROR_ACCESS_DENIED &&
err != ERROR_SHARING_VIOLATION &&
@@ -121,10 +122,9 @@ pgunlink(const char *path)
/* We undefined these above; now redefine for possible use below */
#define rename(from, to) pgrename(from, to)
#define unlink(path) pgunlink(path)
-#endif /* defined(WIN32) || defined(__CYGWIN__) */
-#if defined(WIN32) && !defined(__CYGWIN__) /* Cygwin has its own symlinks */
+#if defined(WIN32)
/*
* pgsymlink support:
@@ -352,4 +352,4 @@ pgwin32_is_junction(const char *path)
}
return ((attr & FILE_ATTRIBUTE_REPARSE_POINT) == FILE_ATTRIBUTE_REPARSE_POINT);
}
-#endif /* defined(WIN32) && !defined(__CYGWIN__) */
+#endif /* defined(WIN32) */
--
2.35.1
