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

Microsoft tools: change relative imports to top-level absolute imports #4431

Merged
merged 1 commit into from
Oct 14, 2023

Conversation

jcbrill
Copy link
Contributor

@jcbrill jcbrill commented Oct 11, 2023

Change the module imports from relative imports to top-level absolute imports for the Microsoft tools.

Moving any of the Microsoft tools that used relative imports to the scons site tools folder would fail on import (i.e., the relative import paths become invalid when the tools are moved).

Internal changes: no test and/or documentation changes necessary.

The first in a sequence of smaller PRs from #4409.

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

… imports for the Microsoft tools.

Moving any of the Microsoft tools that used relative imports to the scons site tools folder would fail on import (i.e., the relative import paths become invalid when the tools are moved).
@mwichmann
Copy link
Collaborator

Hmm, as to the rationale, not sure why things which are part of the SCons distribution need to support being moved out of the distribution.

That said, for the individual tool modules, it does seem okay to import SCons.Tool.MSCommon as this proposes, doing this a relative import is not necessary.

@mwichmann mwichmann added the MSVC Microsoft Visual C++ Support label Oct 12, 2023
@jcbrill
Copy link
Contributor Author

jcbrill commented Oct 13, 2023

Hmm, as to the rationale, not sure why things which are part of the SCons distribution need to support being moved out of the distribution.

To (easily) customize an existing tool outside of the scons source tree (e.g., additional functionality, bug fixes, debugging, etc), The tools can be copied to the site tools folder and modified as desired.

For example, depending on how older msvc versions are excised from the code base with regard to the backend tools, there may be utility for a limited set of users in using ms tools from a previous release of scons (modified if necessary) and bypassing the front-end msvc selection for legacy builds.

@jcbrill
Copy link
Contributor Author

jcbrill commented Oct 13, 2023

An example of a locally customized tool is shown below based on a recent mailing list discussion for the microsoft assembler and the masm tool (quick and dirty, just for fun, and likely bug-riddled).

32-bit targets use 'ml' and 64-bit targets use 'ml64'. However, in recent msvc releases, a 32-bit build will have both 'ml' and 'ml64' on the path. One would want the first eligible program found in the path.

The example below assumes that the MSVC tool has already been configured (i.e., msvc is loaded before masm). Otherwise, more work would need to be done.

A custom masm.py tool that attempts to find the correct binary could be something like the following:

...
import SCons.Defaults
import SCons.Tool
import SCons.Util

ASSuffixes = ['.s', '.asm', '.ASM']
ASPPSuffixes = ['.spp', '.SPP', '.sx']
if SCons.Util.case_sensitive_suffixes('.s', '.S'):
    ASPPSuffixes.extend(['.S'])
else:
    ASSuffixes.extend(['.S'])

ml_progs = ['ml64', 'ml']  # prefer ml64 if none found in generate (exists ignored)

def detect_ml(env, progs):
    program = None
    found = []
    for prog in progs:
        p = env.WhereIs(prog)
        if p:
            found.append((prog, p))
    if found:
        if len(found) == 1:
            program = found[0][0]
        else:
            found_paths = {
                os.path.normcase(os.path.split(t[-1])[0]): t[0]
                for t in found
            }
            env_path = env['ENV']['PATH']
            if SCons.Util.is_String(env_path):
                env_path = env_path.split(os.pathsep)
            for p in env_path:
                p = os.path.normcase(p)
                found_prog = found_paths.get(p, None)
                if not found_prog:
                    continue
                program = found_prog
                break
    return program

def generate(env) -> None:
    """Add Builders and construction variables for masm to an Environment."""
    static_obj, shared_obj = SCons.Tool.createObjBuilders(env)

    for suffix in ASSuffixes:
        static_obj.add_action(suffix, SCons.Defaults.ASAction)
        shared_obj.add_action(suffix, SCons.Defaults.ASAction)
        static_obj.add_emitter(suffix, SCons.Defaults.StaticObjectEmitter)
        shared_obj.add_emitter(suffix, SCons.Defaults.SharedObjectEmitter)

    for suffix in ASPPSuffixes:
        static_obj.add_action(suffix, SCons.Defaults.ASPPAction)
        shared_obj.add_action(suffix, SCons.Defaults.ASPPAction)
        static_obj.add_emitter(suffix, SCons.Defaults.StaticObjectEmitter)
        shared_obj.add_emitter(suffix, SCons.Defaults.SharedObjectEmitter)

    ml = detect_ml(env, ml_progs)

    env['AS']        = ml if ml else ml_progs[0]
    env['ASFLAGS']   = SCons.Util.CLVar('/nologo')
    env['ASPPFLAGS'] = '$ASFLAGS'
    env['ASCOM']     = '$AS $ASFLAGS /c /Fo$TARGET $SOURCES'
    env['ASPPCOM']   = '$CC $ASPPFLAGS $CPPFLAGS $_CPPDEFFLAGS $_CPPINCFLAGS /c /Fo$TARGET $SOURCES'
    env['STATIC_AND_SHARED_OBJECTS_ARE_THE_SAME'] = 1

def exists(env):
    return detect_ml(env, ml_progs)
...

DISCLAIMER: The masm.py tool was not actually tested. The _detect_ml function had limited testing.

@mwichmann
Copy link
Collaborator

I get the idea, I just figure if you're going to make a test copy in a different place anywhere, it doesn't hurt to modify the import there. This wasn't a strong objection, just an objsetvation.

Meanwhile, off the topic of the PR but related to the contents of the previous comment, if we're going to mod masm.py it ought to also be well-behaved in the case of user request, like if someone did:

env = Environment(AS='ml64')

then we ought not to ignore that request (i.e. AS already has a value when the tool is called)

@bdbaddog
Copy link
Contributor

I don't see any harm and certainly some utility in this change so merging.

@bdbaddog bdbaddog merged commit 58159bd into SCons:master Oct 14, 2023
3 of 5 checks passed
@jcbrill jcbrill deleted the jbrill-msvc-toolimports branch October 15, 2023 11:08
@mwichmann mwichmann added this to the 4.6 milestone Oct 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
MSVC Microsoft Visual C++ Support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants