Skip to content

WIP Fix up the ParseFlags regression test #4740

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mwichmann
Copy link
Collaborator

This was failing on a GitHub Windows Runner, for obscure reasons. Reworked a bunch to be more general.

Dropped the whole dance about getting an ordered dict, since with the current base Python version you get that for free.

Initiallly, have taken the test out of the skip list so can see if builds work everywhere other than my environment. If so, I will update with changelog info and other bookkeeping.

Contributor Checklist:

  • I have created a new test or updated the unit tests to cover the new/changed functionality.
  • I have updated CHANGES.txt and RELEASE.txt (and read the README.rst).
  • I have updated the appropriate documentation

This was failing on a GitHub Windows Runner, for obscure reasons.
Reworked a bunch to be more general.

Dropped the whole dance about getting an ordered dict,
since with the current base Python version you get that for free.

Signed-off-by: Mats Wichmann <[email protected]>
@mwichmann mwichmann added the testsuite Things that only affect the SCons testing. Do not use just because a PR has tests. label Jul 4, 2025
@mwichmann
Copy link
Collaborator Author

I added an earlier comment, but don't see it. Appveyor tests failed. There are two pkg-config flavors around (well, three, actually). The original Freedesktop one (https://gitlab.freedesktop.org/pkg-config/pkg-config), and a replacement named pkgconf (https://github.com/pkgconf/pkgconf) which, when installed, also names itself pkg-config to provide compatibility. Distros I checked use pkgconf, but obviously I just checked a few. Only the latter supports the --with-path option, so I can't portably use that option. The Perl reimplementation we get from Strawberry responds to a verion query as it if were the Freedesktop one (0.26, the alternative will answer something like 2.5), but seems to support the --with-path option - which I found by looking at its help, so it's a hybrid. Apparently what's installed in the Appveyor windows image is a version of the Freedesktop version so it reports fail not recognizing the option, while on my own msys2 setup, I'm clearly getting pkgconf. I don't understand that, and don't find any quick explanation.

Is it worth continuing to wrestle with this and find a "portable enough" approach or do we just leave it broken if you happen to have installed Strawberry Perl (and thus skipped on Github CI)? Feel like the couple of hours I spent chasing my tail on this ended up as a waste.

@bdbaddog
Copy link
Contributor

bdbaddog commented Jul 4, 2025

So this work is just to make it work with strawberry perl installed which is only on appveyor at this point?
I think the work so far is enough.
It's not breaking any other platforms as far as we know?

@mwichmann
Copy link
Collaborator Author

mwichmann commented Jul 5, 2025

So this work is just to make it work with strawberry perl installed which is only on appveyor at this point? I think the work so far is enough. It's not breaking any other platforms as far as we know?

Strawberry = only github. And I did all this nice stuff and broke appveyor, because it has some ancient pkg-config that doesn't take the helpful option I chose to use.

@jcbrill
Copy link
Contributor

jcbrill commented Jul 6, 2025

Just a thought, is there any chance that installing the pyconf pkgconf package on Windows could be made to work?

I know it adds a pkg-config.exe to the python script directory which is earlier than the Strawberry perl directory (i.e., pkg-config.bat).

I haven't gotten around to seeing why exactly the test failed with the python version yet. The changes for windows in this PR might be worth testing with python pkgconf it might be easier than working around strawberry perl.

Edit:
pyconf -> pkgconf

@bdbaddog
Copy link
Contributor

bdbaddog commented Jul 6, 2025

I think it's not worth the effort. If we have some test environments which test this functionality (which realistically is unlikely to be used in the field on windows.. ) then that's good enough. I wouldn't spend effort trying to get it working on windows..

@jcbrill
Copy link
Contributor

jcbrill commented Jul 6, 2025

Good. I didn't realize pkgconf requires 3.9+ anyway.

@mwichmann
Copy link
Collaborator Author

It's just that the environment on the AppVeyor 2017 image is too old for the new approach. We could force an update in the msys environment - though I don't know how to do that, actually.

@jcbrill
Copy link
Contributor

jcbrill commented Jul 6, 2025

Quick google result.

Possibly out of luck as VS2017 may be too old.

https://www.msys2.org/docs/ci/

Appveyor

Appveyor provides a MSYS2 installation on all their images under C:\msys64, see https://www.appveyor.com/docs/windows-images-software/

Make sure to use the Visual Studio 2019 image or newer, as the MSYS2 installation on older images is outdated and updating there no longer works.

In case you want to update the MSYS2 installation and install packages you need to update MSYS2 first. For this you need to run the following commands:

# Update MSYS2
C:\msys64\usr\bin\bash -lc "pacman --noconfirm -Syuu"  # Core update (in case any core packages are > outdated)
C:\msys64\usr\bin\bash -lc "pacman --noconfirm -Syuu"  # Normal update

# Then run your code
$env:CHERE_INVOKING = 'yes'  # Preserve the current working directory
$env:MSYSTEM = 'UCRT64'  # Start a 64 bit Mingw environment
C:\msys64\usr\bin\bash -lc "./ci-build.sh"

@mwichmann
Copy link
Collaborator Author

I did suggest earlier that it's time to ditch the 2017 image there, but got pushback.

I'm not sure if it's picking pkg-config from the mingw path or the msys path... or even the cygwin path (I had a debug print reporting this while I worked on the patch, but took it out before pushing). This is the top of the Path reported from that failing run (flipped the slashes for my own sanity):

Path=C://Python37
C://Python37//Scripts
C://ProgramData//chocolatey//bin
C://MinGW//bin
C://MinGW//msys//1.0//bin
C://cygwin//bin
C://msys64//usr//bin
C://msys64//mingw64//bin

@jcbrill
Copy link
Contributor

jcbrill commented Jul 6, 2025

Looks like C:/msys64/usr/bin/pkg-config.EXE:

[00:04:41] 244/1309 ( 18.64%) C:\Python37\python.exe test\CPPDEFINES\pkg-config.py
[00:04:41] C:\projects\scons\scripts\scons.py returned 2
[00:04:41] STDOUT =========================================================================
[00:04:41] scons: Reading SConscript files ...
[00:04:41] 
[00:04:41] STDERR =========================================================================
[00:04:41] Unknown option --with-path=.
[00:04:41] OSError: '"C:/msys64/usr/bin/pkg-config.EXE" --with-path=. --cflags bug' exited 1:
[00:04:41]   File "C:\Users\appveyor\AppData\Local\Temp\1\scons\testcmd.732.1r_bdwar\SConstruct", line 15:
[00:04:41]     env_1.ParseConfig('"C:/msys64/usr/bin/pkg-config.EXE" --with-path=. --cflags bug')
[00:04:41]   File "C:\projects\scons\SCons\Environment.py", line 1850:
[00:04:41]     return function(self, self.backtick(command), unique)
[00:04:41]   File "C:\projects\scons\SCons\Environment.py", line 816:
[00:04:41]     raise OSError(f'{command!r} exited {cp.returncode}')
[00:04:41] 
[00:04:41] 
[00:04:41] FAILED test of C:\projects\scons\scripts\scons.py
[00:04:41] 	at line 753 of C:\projects\scons\testing\framework\TestCommon.py (_complete)
[00:04:41] 	from line 867 of C:\projects\scons\testing\framework\TestCommon.py (run)
[00:04:41] 	from line 485 of C:\projects\scons\testing\framework\TestSCons.py (run)
[00:04:41] 	from line 166 of test\CPPDEFINES\pkg-config.py
[00:04:41] 
[00:04:41] Test execution time: 1.2 seconds

@mwichmann
Copy link
Collaborator Author

sigh. of course it says so in the test itself.... thanks.

@jcbrill
Copy link
Contributor

jcbrill commented Jul 6, 2025

Off-topic: Windows runner 2022 and 2025 environment configuration diagnostics [WIP].

I have found the following diagnostic information useful when trying to figure out how the windows runner environments are configured. Still working dumping the powershell modules.

The attached archive file contains hand-edited log files printing configuration diagnostics for the windows-2022 and windows-2025 runners. This is a work in progress and may not match the SCons test environment yet exactly.

windows-runner-diagnostics-wip01.zip

The ##[group]Run python _jbrill/diagnostics/github-runner/diagnostics.py contains the following sections:

  • Platform - Windows information
  • Environment - All environment variables with paths split into sub-lists.
  • Executables (by name) - Organized by basename with all PATHEXT extensions found on the system path
  • Executables (by path) - Organized by system path folder in system path order (may change to alphabetical).
  • Roots: Subdirectories (one-level) and files under each root.

The ##[group]Run _jbrill/diagnostics/github-runner/vssetup.ps1 contains the following sections:

  • vs_2022_enterprise_release.json - vs bootstrapper export of packages and extensions.
  • vsconfiguration.json - custom export of packages using the VSSetup powershell module

This started with the VS configuration information. It is possible to export the package configuration file and then use that configuration file to install packages on another machine. I haven't actually attempted to try to re-install yet. Unfortunately the export is done from the visual studio installer GUI and can't be done from the command-line without the vs bootstrapper executable. The bootstrapper executable is downloaded based on the installations detected and the vsconfig file is exported. An alternative method using the VSSetup powershell module appears to find all of the packages but does not seem to include installed extensions (which I don't really care about at the moment).

Enjoy.

@mwichmann
Copy link
Collaborator Author

The msys2 project has both mingw-64-pkg-config and mingw-w64-pkgconf. Those are the ones that would go into the generic path (msys64/usr/bin). Since pkgconf was relatively new in 2017 (it only reached 1.0 in late 2016), I'm guessing the install in the 2017 image is the former, and the one in the later images is the latter (I'm not prepared to go digging into historical records of what the setup enshrined in the msys2 "installer" looked like over time).

@jcbrill
Copy link
Contributor

jcbrill commented Jul 6, 2025

VS2017+ comments.

Starting with VS2017, there is no longer a "full" offline installer available (i.e., exe or iso).

The vs bootstrapper is required for each "flavor" of install: enterprise, professional, community, buildtools, and express (2017 only).

It appears that it is possible to use the vs bootstrapper to create an "offline" installation folder and install from the "offline" installation folder on another machine using the vs bootstrapper without connecting to the internet.

The vs bootstrapper links for 2017 are no longer available directly from the MS visual studio pages. However, the direct download links appear to still be working.

If one has an interest in installing VS2017 in the future for testing, creating an offline installation should probably be done sooner rather than later.

@mwichmann
Copy link
Collaborator Author

no, msys2 installer. that's where pkg-config is coming from.

@jcbrill
Copy link
Contributor

jcbrill commented Jul 6, 2025

no, msys2 installer. that's where pkg-config is coming from.

? Unclear what this is referencing.

@mwichmann
Copy link
Collaborator Author

my previous comment

@bdbaddog
Copy link
Contributor

bdbaddog commented Jul 6, 2025

Just skip this test in vs2017 and let's move on?
Seems like a lot effort for something we've never had any complaints about in the environment it's failing in.

@mwichmann mwichmann moved this to In progress in Next Release Jul 19, 2025
@mwichmann mwichmann changed the title Fix up the ParseFlags regression test WIP Fix up the ParseFlags regression test Jul 19, 2025
@mwichmann
Copy link
Collaborator Author

Temporarily marking this work-in-progress till I get the motivation to partially roll back changes so it covers the known CI situations as well as normal Linux dev. That still won't be perfect (no such thing), but should work for us.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testsuite Things that only affect the SCons testing. Do not use just because a PR has tests.
Projects
Status: In progress
Development

Successfully merging this pull request may close these issues.

3 participants