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: quote include directory for resource compiler #4269

Merged
merged 2 commits into from
Jan 27, 2025

Conversation

luau-project
Copy link
Contributor

Description

Fixes #4268

Test

  1. Open a x64 MSVC dev prompt
  2. Execute the commands below (like the steps described [CMake][Visual Studio] Fails to build on source directories containing whitespaces #4268) to see that the issue has been fixed:
    SET MY_SRC_DIR=%TEMP%\zstd fork with spaces
    SET MY_BUILD_DIR=%TEMP%\zstd-fork-build-dir
    SET MY_INSTALL_DIR=%TEMP%\zstd-fork-install-dir
    git clone https://github.com/luau-project/zstd --branch=fix-rc-include "%MY_SRC_DIR%"
    cmake -G "NMake Makefiles" -DCMAKE_BUILD_TYPE=Release --install-prefix "%MY_INSTALL_DIR%" -S "%MY_SRC_DIR%\build\cmake" -B "%MY_BUILD_DIR%"
    cmake --build "%MY_BUILD_DIR%" --config Release
    cmake --install "%MY_BUILD_DIR%" --config Release
  3. The build / install should succeed.

@facebook-github-bot
Copy link

Hi @luau-project!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

@facebook-github-bot
Copy link

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

@Cyan4973 Cyan4973 self-assigned this Jan 26, 2025
@Cyan4973
Copy link
Contributor

I suspect it would be good to have a CI test that reproduces this issue,
so that, in the future, this property remains ensured after every code change.

@luau-project
Copy link
Contributor Author

I suspect it would be good to have a CI test that reproduces this issue, so that, in the future, this property remains ensured after every code change.

Hello @Cyan4973 . I can write a GitHub CI job to test it. Since I don't know the inner details of zstd CI testing, I would need to know which workflow (.yaml file) I should include a test job.

@Cyan4973
Copy link
Contributor

Cyan4973 commented Jan 26, 2025

In .github/dev-short-tests.yml,
there is a test named cmake-build-and-test-check.

The idea would be to either piggy-back on this test to add a "directory with space" condition,
or create nearby another similar one, which can generate the wanted condition.

@luau-project
Copy link
Contributor Author

In .github/dev-short-tests.yml, there is a test named cmake-build-and-test-check.

The idea would be to either piggy-back on this test to add a "directory with space" condition, or create nearby another similar one, which can generate the wanted condition.

I've got a working CI job to build / test zstd on a source directory with spaces. What are your thoughts on this CI job?

  cmake-source-directory-with-spaces:
    runs-on: ${{ matrix.os }}    
    strategy:
      matrix:
        include:
          - os: ubuntu-latest
            generator: "Unix Makefiles"
          - os: windows-latest
            generator: "NMake Makefiles"
          - os: macos-latest
            generator: "Unix Makefiles"
    env:
      SRC_DIR: "source directory with spaces"
    steps:
    - uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # tag=v4.1.1
      with:
        path: "${{ env.SRC_DIR }}"
    - uses: ilammy/msvc-dev-cmd@v1
      if: ${{ matrix.generator == 'NMake Makefiles' }}
    - name: cmake build and test source directory with spaces
      run: |
        cmake -S "${{ env.SRC_DIR }}/build/cmake" -B build -DBUILD_TESTING=ON -G "${{ matrix.generator }}" -DCMAKE_BUILD_TYPE=Release --install-prefix "${{ runner.temp }}/install"
        cmake --build build --config Release
        ctest --test-dir build -C Release
        cmake --install build --config Release

Note

These runs are taking 21m to complete. Should I keep the testing part or remove ctest --test-dir build -C Release to speed it up?

I'm awaiting your review and signal to push the CI job to the file .github/dev-short-tests.yml on this branch.

@Cyan4973
Copy link
Contributor

21 mn seems a bit much for a build test.
That would make it one of the slowest tests (unless you mean 3x 7mn in parallel ?).

For comparison sake, the current cmake-build-and-test-check takes 3 mn.

@Cyan4973
Copy link
Contributor

Also, just minor point,
here is my preferred process for a new CI test:

  • create a branch with the new test
  • on first push to PR, it fails, as it should, because the fix is not yet merged to dev
  • push the fix to dev
  • rebase the test on top of dev => now it succeeds.

@luau-project
Copy link
Contributor Author

21 mn seems a bit much for a build test. That would make it one of the slowest tests (unless you mean 3x 7mn in parallel ?).

For comparison sake, the current cmake-build-and-test-check takes 3 mn.

Yes, they were running in parallel. I removed the line ctest --test-dir build -C Release, and now this job runs on 1m57s.

@luau-project
Copy link
Contributor Author

Also, just minor point, here is my preferred process for a new CI test:

* create a branch with the new test

* on first push to PR, it fails, as it should, because the fix is not yet merged to `dev`

* push the fix to `dev`

* rebase the test on top of `dev` => now it succeeds.

oh, I'm not that skilled on git, specially with rebase. I probably would mess up the history.

I can either push the job (without the testing line) to the proper workflow as a new commit, or you can manipulate and edit the git history, since you are allowed to edit as a maintainer.

@Cyan4973
Copy link
Contributor

I can either push the job (without the testing line) to the proper workflow as a new commit, or you can manipulate and edit the git history, since you are allowed to edit as a maintainer.

That works, I can take care of this.

Copy link
Contributor

@Cyan4973 Cyan4973 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's interesting to note that macos seems to have been immune to this issue,
but Windows definitely wasn't.

@Cyan4973 Cyan4973 merged commit 6a65a43 into facebook:dev Jan 27, 2025
99 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CMake][Visual Studio] Fails to build on source directories containing whitespaces
3 participants