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 2755: msvs tool, msvs tests, and msvc detection fixes #4453

Merged
merged 4 commits into from
Jan 3, 2024

Conversation

jcbrill
Copy link
Contributor

@jcbrill jcbrill commented Dec 28, 2023

Fixes:

  • Fixes Make the VS tool less intrusive of the SCONS_HOME variable #2755: the msvs tool no longer writes the OS environment SCONS_HOME value into the SCons environment when the SCONS_HOME variable already exists in the SCons environment.
  • Update the windows registry keys for detection of Visual Studio 2015 Express ('14.0Exp'): the VS2015 registry key (WDExpress) appears to be different than the registry key (VCExpress) for earlier Visual Studio express versions.
  • Fix the vs-6.0-exec.py test script: the msvs generated project is foo.dsp and the command-line invocation of the Visual Studio development environment program was attempting to build test.dsp.
  • Update the msvs project generation test scripts: the msvs project execution tests could produce a false positive test result when the test executable is correctly built via the SConstruct env.Program() call and the command-line invocation of the Visual Studio development environment program fails.
  • Add method unlink_files to TestCmd class that unlinks a list of files from a specified directory. Add test cases for unlink_files to TestCmdtTests.

No documentation changes were necessary.

Contributor Checklist:

  • I have created a new test or updated the unit tests to cover the new/changed functionality.
  • I have updated CHANGES.txt (and read the README.rst)
  • I have updated the appropriate documentation

Updates:
* Fix issue SCons#2755: the msvs tool no longer writes the OS environment SCONS_HOME value into the SCons environment when the SCONS_HOME variable already exists in the SCons environment.
* Update the windows registry keys for detection of Visual Studio 2015 Express ('14.0Exp'): the VS2015 registry key ('WDExpress') appears to be different than the registry key ('VCExpress') for earlier Visual Studio express versions.
* Fix the vs-6.0-exec.py test script: the msvs generated project is 'foo.dsp' and the command-line invocation of the Visual Studio development environment program was attempting to build 'test.dsp'.
* Update the msvs project generation test scripts: the msvs project execution tests could produce a "false positive" test result when the test executable is correctly built via the SConstruct env.Program() call and the command-line invocation of the Visual Studio development environment program fails.
@jcbrill jcbrill force-pushed the jbrill-msvs-sconshome branch from a6087b5 to f9b68fe Compare December 28, 2023 17:33
@jcbrill jcbrill changed the title MSVS/MSVC: msvs tool, msvs tests, and msvc detection fixes Fix 2755: msvs tool, msvs tests, and msvc detection fixes Dec 28, 2023
@@ -90,6 +90,11 @@

test.run(chdir='sub dir', arguments='.')

for filename in ('foo.exe', 'foo.obj', '.sconsign.dblite'):
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you move this into TestSConsMSVS? rather than copy/pasting it into all the files?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should it be a function that takes a list of files?

Something like:

test.remove_files(['foo.exe', 'foo.obj', 'sconsign.dblite'])

Copy link
Contributor

Choose a reason for hiding this comment

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

Does scons -c not do this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pretty sure it also removes the generated msvs project.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would the following be acceptable?

TestSConsMSVS.py

    def unlink_files(self, filedir, filenames):
        for filename in filenames:
            filepath = self.workpath(filedir, filename)
            if os.path.exists(filepath):
                self.unlink(filepath)

test scripts:

test.unlink_files('sub dir', ['foo.exe', 'foo.obj', '.sconsign.dblite'])

Copy link
Contributor

Choose a reason for hiding this comment

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

does scons -c not remove the files in question?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm pretty sure that scons -c removes the generated msvs project which needs to be built from the command-line with the msdev binary. There will be no project to pass to the command-line after "scons -c" is run.

The sequence is:

  • scons SConstruct (generates msvs project and builds program)
  • delete program binaries
  • pathto\msdev.com foo.dsp to build the program binaries via the generated workspace.

Copy link
Contributor

Choose a reason for hiding this comment

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

gotcha. ok so it's not about cleaning all the build products, but rather removing those which should be built when you have msvs run the created project file.

Ok. I'd add that method to TestCmd.py where there are currently other file handling functions, rather than TestSConsMSVS, as it could be useful for non MSVC tests.

Copy link
Contributor Author

@jcbrill jcbrill Dec 28, 2023

Choose a reason for hiding this comment

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

Changes:

  1. Added rewritten unlink_files to TestCmd class with docstring and functionality consistent with unlink method.
  2. Replaced all msvs executable script deletion loops with single test method call.
  3. Reordered authors alphabetically in CHANGES.txt,

The following are all equivalent:

test.unlink_files('sub dir', ['foo.exe', 'foo.obj', '.sconsign.dblite'])
test.unlink_files(['sub dir'], ['foo.exe', 'foo.obj', '.sconsign.dblite'])
test.unlink_files('', [['sub dir', 'foo.exe'], ['sub dir', 'foo.obj'], ['sub dir', '.sconsign.dblite']])
test.unlink_files([''], [['sub dir', 'foo.exe'], ['sub dir', 'foo.obj'], ['sub dir', '.sconsign.dblite']])
test.unlink_files('.', [['sub dir', 'foo.exe'], ['sub dir', 'foo.obj'], ['sub dir', '.sconsign.dblite']])
test.unlink_files(['.'], [['sub dir', 'foo.exe'], ['sub dir', 'foo.obj'], ['sub dir', '.sconsign.dblite']])

@jcbrill
Copy link
Contributor Author

jcbrill commented Dec 29, 2023

No additional work is planned.

@bdbaddog
Copy link
Contributor

bdbaddog commented Jan 3, 2024

Look good. Merging!

@bdbaddog bdbaddog merged commit 3236d7a into SCons:master Jan 3, 2024
6 of 7 checks passed
@jcbrill jcbrill deleted the jbrill-msvs-sconshome branch January 3, 2024 10:52
@mwichmann mwichmann added this to the 4.7 milestone Jan 9, 2024
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.

Make the VS tool less intrusive of the SCONS_HOME variable
3 participants