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

SCons: Fix silence_msvc implementation errors #91890

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Repiteo
Copy link
Contributor

@Repiteo Repiteo commented May 12, 2024

Fixes #91883

My eyes have been opened to how futile it is to consolidate logic on this silence_msvc nonsense, so stdout is gonna be more explicitly parsed now. Namely, the decoding now happens immediately & each line is checked individually for potential bloat capture. In the interest of keeping this somewhat restrained, the checks are tailored for this repository where appropriate & warnings are given when this is the case.

@jcbrill
Copy link

jcbrill commented May 13, 2024

Potential issues:

  1. The check to see if a user is manually rerouting stdout should probably check stderr as well:

                # Process as normal if the user is manually rerouting stdout.
                for arg in args:
    -               if arg.find(">", 0, 1) != -1 or arg.find("1>", 0, 2) != -1:
    +               if arg.find(">", 0, 1) != -1 or arg.find("1>", 0, 2) != -1  or arg.find("2>", 0, 2) != -1:
                        return old_spawn(sh, escape, cmd, args, env)

    Otherwise, when a user is attempting to manually redirect stderr one of the following would apply:

    • 2>&1

      The user is redirecting command stderr to stdout which is silently ignored.

      Because the temporary file is appended after the argument list, the windows command line becomes:

      cl ... 2>&1 >tempfilename
      

      Anything written to stderr will not be redirected to the temporary file due to the order of the command arguments.

      The following would be required to redirect to stderr to the temporary file:

      cl ... 2>&1 >tempfilename 2>&1
      

      or

      cl ... >tempfilename 2>&1
      

      Effectively, the user is not getting what was expected.

    • 2>>myuserfile.txt or 2>myuserfile.txt

      The user is redirecting command stderr to a file which works as expected.

      However, the filter code is now writing to stderr after the command completes.

      This likely is not what a user would expect as the post-command writes to stderr are not captured in the user's redirected file.

  2. The link filter check is language dependent:

    elif not is_cl and line.startswith("   Creating library"):
    

    If the language is English, the target line looks like:

    link /nologo /OUT:_build001\hello.exe _build001\hello-1.obj _build001\hello-2.obj
       Creating library _build001\hello.lib and object _build001\hello.exp
    

    If the language is German, the target line looks like:

    link /nologo /OUT:_build001\hello.exe _build001\hello-1.obj _build001\hello-2.obj
       Bibliothek "_build001\hello.lib" und Objekt "_build001\hello.exp" werden erstellt.
    

    There are a number of ways to proceed but all are likely infallible in one way or another.

    A simple-ish test would be "starts with 3 spaces and contains '.lib' and contains '.exp'".

    A more involved test involves finding the build artifact name from the link command and searching for the actual lib and exp file names in the argument. A dynamically constructed regex would be useful.

Postscript

I realize that the stream checks are similar to those in the SCons platform win32.py code.

IMHO, using arg.find() != -1 is a bit obtuse for simply checking the start of a string.

Unless I'm missing something, which is always possible, the following are equivalent:

  • Using startswith is easier to read:

            for arg in args:
              if arg.startswith(">") or arg.startswith("1>") or arg.startswith("2>"):
                  return old_spawn(sh, escape, cmd, args, env)
    
  • Using a regular expression is more compact:

    re_redirect_stream = re.compile(r'^[12]?>')
    ...
            for arg in args:
              if re_redirect_stream.match(arg):
                  return old_spawn(sh, escape, cmd, args, env)
    

@Repiteo
Copy link
Contributor Author

Repiteo commented May 13, 2024

Hot damn, you're on a roll with these insights! Went ahead and expanded the logic to incorporate those points & a few other improvements:

  • Changed the output check to your regex, so it also checks for stderr as well.
  • Link capture check changed to a simple regex that looks for starting 3 spaces & a 4th character that isn't whitespace. This is hopefully specific enough that it won't cause false positives, but even if it does…
  • Captured output is now rerouted to msvc_capture.log. That way, even if false positives occur, the values captured will be preserved and readily accessible (provided you don't start another build).
  • As it's outside the scope of this PR, but still a valid issue to look into: a FIXME was added above setup_msvc_auto that warns of potential overwrite issues. Didn't want it to get buried after this PR "fixes" that issue.

@jcbrill
Copy link

jcbrill commented May 13, 2024

Looks good.

Suggestions:

  • The regexes only need to be compiled once: suggest moving outside of spawn_capture function.

Optional enhancements, feel free to ignore:

  • A regex ending the line with the known c/c++ suffixes could be used for the compiler. For example, assume a line ending with a c/c++ suffix is the capture line:

    re_cl_filename = re.compile('^.+\.(c|cc|cpp|cxx|c[+]{2})$', re.IGNORECASE)
    

    The regex above has not been tested.

  • If the link search includes the .lib and .exp file name sequence, there would be fewer false positives. For example:

    re_link_capture = re.compile(r'\s{3}\S.+\s(?:"[^"]+.lib"|\S+.lib)\s.+\s(?:"[^"]+.exp"|\S+.exp)')
    

    Explanation:

    • 3 spaces
    • 1 non-space
    • 1 or more of characters
    • 1 space
    • either:
      • quoted string of 1 or more characters ending with .lib (e.g., "_build001\hello.lib")
      • 1 or more non-space characters ending with .lib (e.g., _build001\hello.lib)
    • 1 space
    • 1 or more characters
    • 1 space
    • either:
      • quoted string of 1 or more characters ending with .exp (e.g., "_build001\hello.exp")
      • 1 or more non-space characters ending with .exp (e.g., _build001\hello.exp)

    It could probably use some more testing. I was using something similar in my test implementation.

@Repiteo
Copy link
Contributor Author

Repiteo commented May 13, 2024

Did a few fresh builds & everything's working as expected!

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.

SCons: silence_msvc implementation issues
3 participants