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

[ci] fix MSVC warning about builds in temp directory #6573

Merged
merged 4 commits into from
Jul 29, 2024
Merged

Conversation

StrikerRUS
Copy link
Collaborator

@StrikerRUS StrikerRUS commented Jul 26, 2024

Currently this fix is applied only to R-package builds.
This PR proposes adding this fix to all Windows builds.

C:\Program Files (x86)\MSBuild\Microsoft.Cpp\v4.0\V140\Microsoft.CppBuild.targets(400,5): warning MSB8029: The Intermediate directory or Output directory cannot reside under the Temporary directory as it could lead to issues with incremental build.

https://ci.appveyor.com/project/guolinke/lightgbm/builds/50283647/job/bo2wb8ejurn0b0k6#L798

@StrikerRUS
Copy link
Collaborator Author

Checked all Windows builds: no warning.

@StrikerRUS StrikerRUS marked this pull request as ready for review July 26, 2024 18:00
@jameslamb jameslamb changed the title [ci] fix MSVS warning about builds in temp directory [ci] fix MSVC warning about builds in temp directory Jul 26, 2024
jameslamb
jameslamb previously approved these changes Jul 26, 2024
Copy link
Collaborator

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

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

Thanks! I agree, this should have always been in test_windows.ps1 and not specific to the R package.

I've restarted the failing CI jobs for you.

I also edited the title to "MSVC" instead of "MSVS". I think it's more common to use "MSVC" when referring to the compiler and "MSVS" when referring to the IDE. That is typically what we've done here in LightGBM. But please change it back if you disagree, I don't think the distinction is too important.

@StrikerRUS
Copy link
Collaborator Author

Hmm, some builds failed with the following error:

        # check that model text has all expected param entries
        for param_str in all_param_entries:
>           assert param_str in model_txt_from_file
E           AssertionError: assert '[force_(col|row)_wise: 1]' in ...

I guess it can be somehow related to persistent mounts across builds (#6465, #6416)... Will investigate this.

@StrikerRUS
Copy link
Collaborator Author

Hmm, some builds failed with the following error:

Wait! They are all Linux jobs, not Windows ones... 🤔

@StrikerRUS
Copy link
Collaborator Author

Hmm, some builds failed with the following error:

Ok, found the root cause!

Jobs are failing because lightgbm is installed from PyPI not from source there.
#6574 fixes this problem. I'll mark that PR as blocking.

@StrikerRUS StrikerRUS dismissed jameslamb’s stale review July 29, 2024 16:47

Please check one more time - I added initial cleanup for the directory

Copy link
Collaborator

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

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

This looks great, thanks!

Hopefully THIS is the last "things left behind in the build directory" issue we have 😂

@jameslamb jameslamb merged commit b33f412 into master Jul 29, 2024
45 checks passed
@jameslamb jameslamb deleted the vs-warning branch July 29, 2024 19:11
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.

2 participants