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

Linking for mingw/x86_64 on MSYS2 fails #1078

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

Conversation

feiyunw
Copy link
Contributor

@feiyunw feiyunw commented Mar 31, 2023

This PR closes #1059:

  • subprocess.Popen() seems to have some deadlock problem. Use subprocess.run() as the document recommends.
  • Some improvement was made to speed up the "ar" command execution. I confirmed that a command of 8149 bytes works with the MSYS2 MinGW 64-bit shell, while 8171 doesn't.

FYI: subprocess

@feiyunw feiyunw requested a review from a team as a code owner March 31, 2023 16:20
@Calinou Calinou added bug This has been identified as a bug platform:windows labels Mar 31, 2023
@feiyunw
Copy link
Contributor Author

feiyunw commented Apr 2, 2023

Command line length=8153 is OK, while 8159 is broken.

@bruvzg
Copy link
Member

bruvzg commented Apr 2, 2023

Godot scons config is using almost the same code and seems to be working fine (but I guess godot-cpp is linking more file, so this might be the issue). If MSYS have 8159 line limits, the same probably should be done in the main repo.

@feiyunw
Copy link
Contributor Author

feiyunw commented Apr 4, 2023

For my curiosity about subprocess.run() implementation, in Python-3.10.10\Lib\subprocess.py:

def run(*popenargs,
        input=None, capture_output=False, timeout=None, check=False, **kwargs):
...
    with Popen(*popenargs, **kwargs) as process:
        try:
            stdout, stderr = process.communicate(input, timeout=timeout)
        except TimeoutExpired as exc:
            process.kill()
            if _mswindows:
                # Windows accumulates the output in a single blocking
                # read() call run on child threads, with the timeout
                # being done in a join() on those threads.  communicate()
                # _after_ kill() is required to collect that and add it
                # to the exception.
                exc.stdout, exc.stderr = process.communicate()
            else:
                # POSIX _communicate already populated the output so
                # far into the TimeoutExpired exception.
                process.wait()
            raise
        except:  # Including KeyboardInterrupt, communicate handled that.
            process.kill()
            # We don't call process.wait() as .__exit__ does that for us.
            raise
        retcode = process.poll()
        if check and retcode:
            raise CalledProcessError(retcode, process.args,
                                     output=stdout, stderr=stderr)
    return CompletedProcess(process.args, retcode, stdout, stderr)

@feiyunw
Copy link
Contributor Author

feiyunw commented Apr 4, 2023

Godot scons config is using almost the same code and seems to be working fine (but I guess godot-cpp is linking more file, so this might be the issue). If MSYS have 8159 line limits, the same probably should be done in the main repo.

Agreed. Better with the whole godot-cpp/tools solution, or a quick fix if somebody reports a similar problem.

@feiyunw
Copy link
Contributor Author

feiyunw commented Apr 4, 2023

Command line length=8158 is the maximum, and 8159 is broken.

@dsnopek dsnopek added the topic:buildsystem Related to the buildsystem or CI setup label Oct 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This has been identified as a bug platform:windows topic:buildsystem Related to the buildsystem or CI setup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Linking for mingw/x86_64 on MSYS2 fails
4 participants