Skip to content
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

fix: EOL problems running CMake tests on Windows #63

Merged
merged 7 commits into from
Jun 30, 2024

Conversation

luau-project
Copy link
Contributor

Description

I have improved cminpack build with CMake on Windows. In the actual behavior when building with CMake, in order for the tests to pass, you have to two ways:

  1. enforce git config --global core.autocrlf true before checking out the source code from github;
  2. download the released tarball and convert manually LF (\n) to CRLF (\r\n) all the reference test files (*.ref) found at examples/ref/

In the new behavior, there is no need to perform LF to CRLF conversion anymore.

How

Tests

To verify that my proposed changes were correct, I discarded the entries git config --global core.autocrlf true in the builds at CI services, which can be found at .github/workflows/windows-visual-studio-cminpack-install.yaml and appveyor.yml.

Bonus

I simplified the build on AppVeyor CI appveyor.yml.

devernay
devernay previously approved these changes Jun 28, 2024
@devernay devernay dismissed their stale review June 28, 2024 17:47

wait for CI success

@luau-project
Copy link
Contributor Author

Update

On my branch, I was able to port MSYS2 CI to github. Github is much faster, because jobs run in parallel.

@luau-project
Copy link
Contributor Author

Update

@devernay

In the current state, which I consider to be ready to merge, I removed the AppVeyor CI integration (deleted appveyor.yml) in favor of Github CI, because it is faster.

Rest assured that CI is now better, which you can check on my branch https://github.com/luau-project/cminpack/tree/fix-cmake-eol-on-tests Github actions (building correctly 49 different tests), including 24 tests with MSYS2 toolchains.

Important

In order to cancel AppVeyor CI, you have to log in on your account there, and delete the cminpack project on the project settings, otherwise it will continue to fail on each push.

@luau-project
Copy link
Contributor Author

Update

Merged master, and restored back AppVeyor file for the checks to pass here.

Important

However, it is still highly recommended to turn off the AppVeyor CI removing the project on their website, because:

  • Github workflows is already setup for MSYS2 and MSVC
  • AppVeyor runs slower than Github, because Github jobs run in parallel
  • AppVeyor can fail (false-positive) while downloading packages or updates from MSYS2 due a great load on MSYS2 servers.

@devernay
Copy link
Owner

LGTM!

@devernay devernay merged commit b061f86 into devernay:master Jun 30, 2024
50 checks passed
@luau-project luau-project deleted the fix-cmake-eol-on-tests branch July 1, 2024 08:28
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