Skip to content

Cygwin: align testsuite tmp directory with runtime path resolution - #343

Open
aswin-mcw wants to merge 1 commit into
msys2:msys2-3.6.10from
aswin-mcw:fix-cygwin-testsuite-tmpdir
Open

Cygwin: align testsuite tmp directory with runtime path resolution#343
aswin-mcw wants to merge 1 commit into
msys2:msys2-3.6.10from
aswin-mcw:fix-cygwin-testsuite-tmpdir

Conversation

@aswin-mcw

Copy link
Copy Markdown

Cygwin: align testsuite tmp directory with runtime path resolution

Summary

While investigating Cygwin testsuite failures in MSYS2, I found that the installation root was being resolved one directory too far back in my build layout.

In init_cygheap::init_installation_root() (winsup/cygwin/mm/cygheap.cc), the __MSYS__ logic strips three directory levels in total when determining the installation root. For my test environment, this resulted in incorrect path resolution.

The testsuite was also creating temporary files under ${builddir}/testinst/tmp. Changing it to use ${builddir}/tmp aligns the test layout with the runtime's expected path resolution.

-$(MKDIR_P) ${builddir}/testinst/tmp
+$(MKDIR_P) ${builddir}/tmp

-rm -rf ${builddir}/testinst/tmp
+rm -rf ${builddir}/tmp

Results

After making this change, the winsup testsuite results improved significantly in my environment:

  • Before: ~148 passing tests out of 282
  • After: ~258 passing tests out of 282

This suggests that a large number of failures were caused by path resolution and test layout issues rather than individual runtime failures.

@dscho

dscho commented Jul 28, 2026

Copy link
Copy Markdown
Collaborator

I have to admit that I am a bit puzzled by this. As per this comment, the MSYS2 runtime is expected to have one extra level relative to the Cygwin runtime, not one less: The cygwin1.dll is supposed to be in /bin/, whereas the MSYS2 runtime wants to live in /usr/bin/.

@aswin-mcw
aswin-mcw force-pushed the fix-cygwin-testsuite-tmpdir branch from 4831fa6 to 1324578 Compare July 29, 2026 10:01
aswin-mcw added a commit to Multicorewareinc/msys2-runtime that referenced this pull request Jul 29, 2026
…/bin

The MSYS2 runtime resolves its installation root by stripping three levels
from the DLL path (<root>/usr/bin/msys-2.0.dll -> <root>).  Stage the test
DLL and point runtime_root at testinst/usr/bin so init_installation_root()
resolves to testinst, /bin and /usr/bin map to testinst/usr/bin, and /tmp
stays testinst/tmp.  (The winsup Makefile side of this lives in the testsuite
PR msys2#343; the CI stages the DLL itself, so only cygwin.yml + the testsuite
Makefile runtime_root/helpers are needed here.)
@aswin-mcw

Copy link
Copy Markdown
Author

I have to admit that I am a bit puzzled by this. As per this comment, the MSYS2 runtime is expected to have one extra level relative to the Cygwin runtime, not one less: The cygwin1.dll is supposed to be in /bin/, whereas the MSYS2 runtime wants to live in /usr/bin/.

Agreed — the "Back two folders to get root as we have all stuff in usr subfolder" logic you linked is exactly right, and it's not what I'm touching. MSYS2 keeps the runtime one level deeper (/usr/bin/msys-2.0.dll vs Cygwin's /bin/cygwin1.dll), so init_installation_root() correctly strips that extra usr level.

The bug was that the testsuite didn't reproduce that layout: it staged the test DLL and the sh/sleep/ls helpers under testinst/bin (the Cygwin 2-level layout, no usr). So the runtime's 3-level strip had no usr to consume and overshot the installation root to the parent of testinst (${builddir}) — which is why /tmp resolved to ${builddir}/tmp instead of testinst/tmp. My earlier testinst/tmp → ${builddir}/tmp change was just chasing that overshot root, so you're right that it was the wrong direction.

I've replaced it with a layout fix so the test tree mirrors a real MSYS2 install:

winsup/cygwin/Makefile.am — stage the test DLL under testinst/usr/bin (was testinst/bin).
winsup/testsuite/Makefile.am — point runtime_root and the sh/sleep/ls helpers at testinst/usr/bin; leave testinst/tmp unchanged.
Now the strip lands on testinst, /bin and /usr/bin both map to testinst/usr/bin, and /tmp stays testinst/tmp — no path hack, and the test env finally has the /usr/bin level the runtime assumes. With the matching CI staging change in #344, the winsup testsuite goes from ~148/282 to 264/282 .

Mirror the real MSYS2 layout so init_installation_root() resolves to testinst.
MSYS2 strips three levels from the DLL path (<root>/usr/bin/msys-2.0.dll -> <root>);
with the DLL and helpers under testinst/bin the extra strip overshot the root to
${builddir}, so /tmp resolved to ${builddir}/tmp instead of testinst/tmp.  Put the
test DLL and the sh/sleep/ls helpers under testinst/usr/bin and point runtime_root
there; / -> testinst, /bin and /usr/bin -> testinst/usr/bin, /tmp stays testinst/tmp.
@dscho
dscho force-pushed the fix-cygwin-testsuite-tmpdir branch from 08eaae8 to b3059f2 Compare July 29, 2026 16:27
@dscho
dscho changed the base branch from msys2-3.6.9 to msys2-3.6.10 July 29, 2026 16:27
@dscho

dscho commented Jul 29, 2026

Copy link
Copy Markdown
Collaborator

@aswin-mcw I opened a PR to fix that (new) build failure you got: #355. I merged that, rebased your branch on top and changed the target branch to the now-current default branch.

@aswin-mcw

Copy link
Copy Markdown
Author

@aswin-mcw I opened a PR to fix that (new) build failure you got: #355. I merged that, rebased your branch on top and changed the target branch to the now-current default branch.

Thanks a lot, @dscho — appreciate you fixing the build failure in #355, merging it, and rebasing/retargeting this branch for me. I've pulled the rebased branch and confirmed it builds and the winsup testsuite still passes at the expected rate (264/282) against the new default branch, and CI here looks green now. Let me know if there's anything else you'd like me to adjust before merge.

@dscho

dscho commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator

@aswin-mcw I'd like to understand those failures first, to see whether we can fix them or not.

The file mode related ones most likely need a /tmp/ that is mounted with acl (instead of the implicit noacl as of MSYS2's 67e0663#diff-0952970e73771f7d9057ddbdd172956d54bc76b4dc3ac3f7c98cba1b0a547671R44-R553), for example, and the msgtest one only fails in that testinst for me, when I run it interactively (after starting a cygserver in the same MSYS2 environment), it succeeds.

@dscho

dscho commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator

In other words, I'd like to proceed at #344 (comment) (i.e. have a single PR that fixes the Cygwin testsuite for MSYS2 and runs it as part of CI).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants