Skip to content

Fix cross-device rename failure#593

Merged
MarkCallow merged 9 commits into
masterfrom
issue581
Jun 20, 2022
Merged

Fix cross-device rename failure#593
MarkCallow merged 9 commits into
masterfrom
issue581

Conversation

@MarkCallow

@MarkCallow MarkCallow commented Jun 17, 2022

Copy link
Copy Markdown
Collaborator

by creating the temporary file in the same directory as the file to be overwritten.

Includes:

  • Add ktxsc tests
  • Improve ktxsc docs
  • Simplify toktx-tests.cmake script
  • Simplify reference image generation scripts
  • Regen skybox_zstd.ktx2 reference with default version numbers.
  • Run tests in Mingw build
  • Fix on_failure.ps1 to work for both Appveyor and GitHub Actions and to use intended curl.

Fixes #581

@MarkCallow

Copy link
Copy Markdown
Collaborator Author

@Honeybunch I need your assistance again. In this PR I modified the Mingw build to run the tests. All but one test are successful. The one that is failing is failing at this call to fopen. The reported failure in the test log is

ktxsc could not open output file "ktxsc.skybox_zstd.ktx2". Invalid argument

The test is ktxsc-cmp-compress-explicit-output.

It runs fine on Windows when ktxsc is built with MSVC and fine on Linux and macOS. The MS docs for fopen say that EINVAL is returned if file name pointer or mode is NULL. Both are non-null here. There is a very similar call here in toktx that is working. All the toktx tests are passing. The only difference is the mode is lacking the x. The MS docs show x as a valid mode character. I have tried searching for documentation or anything else that would indicate that Mingw's fopen has different behavior than Windows'. I have found nothing.

So I'm stumped and since I don't have a Mingw environment set up, I can't readily debug. Please see if you can locate the problem.

@Honeybunch

Copy link
Copy Markdown
Contributor

Yep I was able to repro this locally. I'll take a look at this when I have some free time

@Honeybunch

Copy link
Copy Markdown
Contributor

I tried removing the 'x' component of the mode on that fopen call and yeah the test passed. Must be some strange behavior in mingw's fopen implementation with the 'x' mode setting.

@Honeybunch

Copy link
Copy Markdown
Contributor

Everything I can find about any recent mingw build is that they don't actually re-implement fopen. At least no where that I could find source code for it. I searched through a source archive from the official source forge and I just see msvcrt .def files that I think are just being used to mark linkage in the actual win32 dlls. So I'm extremely unsure why this would fail on mingw only. The MS docs show that the x mode causes failure if the target file already exists. Could it be that a different call is not deleting a test file on disk?

@Honeybunch

Copy link
Copy Markdown
Contributor

So I have this small test program:

#include <stdio.h>
#include <windows.h>
#include <errno.h>

int main()
{
    printf("Trying to write garbage.txt\n");
    if (!fopen("garbage.txt", "wxb"))
    {
        printf("errno was - %d?\n", errno);
        printf("GetLastError was - %ld?\n", GetLastError());
        return 1;
    }
    printf("Done\n");

    return 0;
}

This crashes with errno 22 when compiled with gcc on windows but a clang built version runs fine... I wonder if this has anything to do with _UNICODE

@MarkCallow

MarkCallow commented Jun 18, 2022

Copy link
Copy Markdown
Collaborator Author

Thank you for the investigation.

This crashes with errno 22 when compiled with gcc on windows but a clang built version runs fine... I wonder if this has anything to do with _UNICODE

No. Your test program is calling fopen which is the ordinary character version of the function. _UNICODE only affects whether _tfopen morphs into fopen or _wfopen. _UNICODE is not set when building the KTX tools.

When you say "compiled with gcc", is that any gcc or specifically what is included in Mingw?

The setup-mingw CI step prints this message

You have mingw v8.1.0 installed. Version 11.2.0.07112021 is available based on your source(s).

Could the problem have been fixed in one of the more recent versions?

I just found this Linux fopen man page which does not have x listed as a valid mode character and says the function returns EINVAL if an invalid character is found in mode. However NOTES near the bottom of the page has a section Glibc notes which lists x as a GNU C library extension. There is no indication that you need to do anything to enable those extensions so I don't understand why your test program fails when compiled with gcc.

The ktxsc test passes on the CI build done with GCC/Ubuntu.

@Honeybunch

Copy link
Copy Markdown
Contributor

All gcc on windows that target windows itself use mingw IIRC but yeah using mingw 11.2 from scoop. The clang I use from llvm for windows targets msvcrt directly so that must be the distinction. I just can't find the impl difference between mingw and msvc.

The build machine prints that because thats a message from choco as it installs that newer 11.2 version. Remember that the older 8.1 failed to build KTX at all in the first place.

I guess for now discarding that 'x' flag is a fine solution if it isn't critical behavior. Maybe this could just be done instead with a c++ api or perhaps the behavior of the 'x' flag be implemented outside the callsite manually with a comment explaining why?

@MarkCallow

Copy link
Copy Markdown
Collaborator Author

The build machine prints that because thats a message from choco as it installs that newer 11.2 version. Remember that the older 8.1 failed to build KTX at all in the first place.

Ahh. Thanks. The message is very poorly expressed. I did remember you had some issue but did not remember the details.

I guess for now discarding that 'x' flag is a fine solution if it isn't critical behavior. Maybe this could just be done instead with a c++ api or perhaps the behavior of the 'x' flag be implemented outside the callsite manually with a comment explaining why?

I want to prevent people accidentally overwriting an existing file. By choosing the explicit output option -o they've already indicated they don't want to overwrite the file they are supercompressing. It is very annoying that Mingw does not support this otherwise widely supported feature. The fix I'm thinking of is that if EINVAL is set after the call I will try to open the file for reading to verify it doesn't exist and then open it without x in the mode. This way the fix will work for any version of gcc or Linux that does not support x.

@MarkCallow MarkCallow changed the title Fix cross-device rename failure. Fix cross-device rename failure Jun 20, 2022
@MarkCallow MarkCallow merged commit f020c1b into master Jun 20, 2022
@MarkCallow MarkCallow deleted the issue581 branch June 20, 2022 12:26
KaperD pushed a commit to KaperD/KTX-Software that referenced this pull request Feb 21, 2024
by creating the temporary file in the same directory as the file to be overwritten.

Includes:

    Add ktxsc tests.
    Improve ktxsc docs.
    Simplify toktx-tests.cmake script.
    Simplify reference image generation scripts.
    Regen skybox_zstd.ktx2 reference with default version numbers.
    Run tests in Mingw build.
    Fix on_failure.ps1 to work for both Appveyor and GitHub Actions and to use intended curl.
    Workaround lack of 'x' mode in Mingw fopen.

Fixes KhronosGroup#581.
KaperD pushed a commit to KaperD/KTX-Software that referenced this pull request Feb 22, 2024
by creating the temporary file in the same directory as the file to be overwritten.

Includes:

    Add ktxsc tests.
    Improve ktxsc docs.
    Simplify toktx-tests.cmake script.
    Simplify reference image generation scripts.
    Regen skybox_zstd.ktx2 reference with default version numbers.
    Run tests in Mingw build.
    Fix on_failure.ps1 to work for both Appveyor and GitHub Actions and to use intended curl.
    Workaround lack of 'x' mode in Mingw fopen.

Fixes KhronosGroup#581.
KaperD pushed a commit to KaperD/KTX-Software that referenced this pull request Feb 22, 2024
by creating the temporary file in the same directory as the file to be overwritten.

Includes:

    Add ktxsc tests.
    Improve ktxsc docs.
    Simplify toktx-tests.cmake script.
    Simplify reference image generation scripts.
    Regen skybox_zstd.ktx2 reference with default version numbers.
    Run tests in Mingw build.
    Fix on_failure.ps1 to work for both Appveyor and GitHub Actions and to use intended curl.
    Workaround lack of 'x' mode in Mingw fopen.

Fixes KhronosGroup#581.
KaperD pushed a commit to KaperD/KTX-Software that referenced this pull request Feb 22, 2024
by creating the temporary file in the same directory as the file to be overwritten.

Includes:

    Add ktxsc tests.
    Improve ktxsc docs.
    Simplify toktx-tests.cmake script.
    Simplify reference image generation scripts.
    Regen skybox_zstd.ktx2 reference with default version numbers.
    Run tests in Mingw build.
    Fix on_failure.ps1 to work for both Appveyor and GitHub Actions and to use intended curl.
    Workaround lack of 'x' mode in Mingw fopen.

Fixes KhronosGroup#581.
KaperD pushed a commit to KaperD/KTX-Software that referenced this pull request Feb 22, 2024
by creating the temporary file in the same directory as the file to be overwritten.

Includes:

    Add ktxsc tests.
    Improve ktxsc docs.
    Simplify toktx-tests.cmake script.
    Simplify reference image generation scripts.
    Regen skybox_zstd.ktx2 reference with default version numbers.
    Run tests in Mingw build.
    Fix on_failure.ps1 to work for both Appveyor and GitHub Actions and to use intended curl.
    Workaround lack of 'x' mode in Mingw fopen.

Fixes KhronosGroup#581.
richgel999 pushed a commit to BinomialLLC/KTX-Software-Binomial-Fork that referenced this pull request Mar 9, 2026
by creating the temporary file in the same directory as the file to be overwritten.

Includes:

    Add ktxsc tests.
    Improve ktxsc docs.
    Simplify toktx-tests.cmake script.
    Simplify reference image generation scripts.
    Regen skybox_zstd.ktx2 reference with default version numbers.
    Run tests in Mingw build.
    Fix on_failure.ps1 to work for both Appveyor and GitHub Actions and to use intended curl.
    Workaround lack of 'x' mode in Mingw fopen.

Fixes KhronosGroup#581.
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.

ktxsc: fails with Invalid cross-device link

2 participants