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

MinGW support #36

Closed
wants to merge 6 commits into from
Closed

MinGW support #36

wants to merge 6 commits into from

Conversation

isuruf
Copy link
Contributor

@isuruf isuruf commented Sep 19, 2018

No description provided.

On windows the temp file cannot be opened a second time while
it is still opened by python. Close the file in python before
asking the compiler to read it.

See https://docs.python.org/3.7/library/tempfile.html#tempfile.NamedTemporaryFile
@isuruf isuruf mentioned this pull request Sep 19, 2018
try:
compiler.compile([fname], extra_postargs=[flagname])
except setuptools.distutils.errors.CompileError:
return False
Copy link
Member

Choose a reason for hiding this comment

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

What is the reason for not deleting the temporary file and moving this out of the context manager?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On windows the temp file cannot be opened a second time while
it is still opened by python. Close the file in python before
asking the compiler to read it.

See https://docs.python.org/3.7/library/tempfile.html#tempfile.NamedTemporaryFile

Copy link
Member

Choose a reason for hiding this comment

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

Ok it makes sense.

Copy link
Member

Choose a reason for hiding this comment

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

But in this case, the file should be deleted later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about something like below, so that I don't have to worry about cleaning up stuff?

    import tempfile, os
    with tempfile.TemporaryDirectory() as d:
        fname = os.path.join(d, 'main.cpp')
        with open(fname, 'w') as f:
            f.write('int main (int argc, char **argv) { return 0; }')
        try:
            compiler.compile([fname], extra_postargs=[flagname])
        except setuptools.distutils.errors.CompileError:
            return False
    return True

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TemporaryDirectory is not in py 2.7. Reverting to deleting the file

@darioizzo
Copy link

Has anyone tried to build this in MinGW? I see the Appveyor VMs use MSVC and conda ...

@darioizzo
Copy link

Just to let you guys know that on my mingw system the pybind11 minimal example compiles fine without this PR.

@isuruf
Copy link
Contributor Author

isuruf commented Feb 11, 2019

Just to let you guys know that on my mingw system the pybind11 minimal example compiles fine without this PR.

Yes, but does it work with MinGW 5.x or 4.x series which defaults to C++98?

@isuruf
Copy link
Contributor Author

isuruf commented Feb 20, 2019

@SylvainCorlay, can you review again?

@SylvainCorlay
Copy link
Member

This looks good to me. The build failures seem unrelated.

@isuruf
Copy link
Contributor Author

isuruf commented Feb 28, 2019

Ping

@wjakob wjakob force-pushed the master branch 2 times, most recently from fe696bc to 08a2f37 Compare June 25, 2019 14:50
@isuruf
Copy link
Contributor Author

isuruf commented Apr 9, 2020

This is ready for review now.

wjakob added a commit that referenced this pull request Apr 26, 2020
@wjakob
Copy link
Member

wjakob commented Apr 26, 2020

Rebased and merged in 187e874, can you double-check that it's okay?

@wjakob wjakob closed this Apr 26, 2020
@isuruf
Copy link
Contributor Author

isuruf commented Apr 26, 2020

#55

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.

4 participants