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 to use UTF-8 while reading XML manifest file. #3478

Merged
merged 4 commits into from Nov 25, 2018

Conversation

Projects
None yet
2 participants
@Suzumizaki
Copy link
Contributor

commented Apr 23, 2018

Otherwise, PyInstaller cannot make the package which starts <Non-ASCII-named>.py.
Of course, this is the issue under Windows.

@htgoebel
Copy link
Member

left a comment

Thanks for this pull-request. I now found time for reviewing.

Show resolved Hide resolved PyInstaller/utils/win32/winmanifest.py Outdated
Show resolved Hide resolved tests/functional/test_updating_manifest.py Outdated
Show resolved Hide resolved tests/functional/test_updating_manifest.py
Show resolved Hide resolved tests/functional/test_updating_manifest.py
Show resolved Hide resolved tests/functional/test_updating_manifest.py Outdated
Show resolved Hide resolved tests/functional/test_updating_manifest.py Outdated
@htgoebel
Copy link
Member

left a comment

Thanks for bringing back the test :-) Sorry for not spotting my confusion earlier.

@htgoebel

This comment has been minimized.

Copy link
Member

commented Jul 9, 2018

Please revert the last merge you did. There are 73 changed files now, which I can not keep track of anymore. Try rebasing you code on current develop head.

@Suzumizaki

This comment has been minimized.

Copy link
Contributor Author

commented Jul 12, 2018

Sorry to my panic with this pull request.

And I have another question about test.

You said XML manifest related code should be tested not only under Microsoft Windows but also another environments. But today, Travis CI reports like below:

    ==================================== ERRORS ====================================
    _________ ERROR collecting tests/functional/test_updating_manifest.py __________
    ImportError while importing test module '/home/travis/build/pyinstaller/pyinstaller/tests/functional/test_updating_manifest.py'.
    Hint: make sure your test modules/packages have valid Python names.
    Traceback:
    tests/functional/test_updating_manifest.py:14: in <module>
        from PyInstaller.utils.win32 import winmanifest
    PyInstaller/utils/win32/winmanifest.py:97: in <module>
        from PyInstaller.utils.win32 import winresource
    PyInstaller/utils/win32/winresource.py:19: in <module>
        from ...compat import pywintypes, win32api
    E   ImportError: cannot import name pywintypes

I think this means that utils/win32/winmanifest.py (which is imported by test_updating_manifest.py) is intended to be used only under Windows. How should I do next?

@htgoebel

This comment has been minimized.

Copy link
Member

commented Oct 2, 2018

Sorry for not answering earlier, I'm very busy in my day job.

Since this manifest-code is Windows only, the test can safely be restricted to windows only, too. For this

  • add from PyInstaller.utils.tests import skipif_win to the imports
  • decorate the test with @skipif_win.

If tests pass, please clean up the pull-request - which in the case means: rebase on current deveop head and squash into a single commit. Please reword the commit message to comply to our Commit Message Rules.

You also need to submit a changelog entry so our users can learn about your change. (This is a new requirement since last September to speed up releasing.)

When updating a pull-request, you can simply (force) push the updated branch to github again. This will automatically update the pull-request (which follows the branch, not the commit). So you do not need to close the pull-request and open a new one. This also has the benefit that the discussion history is kept. For detailed instructions please read Updating a Pull-Request in the manual.

Thanks.

@Suzumizaki

This comment has been minimized.

Copy link
Contributor Author

commented Oct 7, 2018

Sorry, I remember my fault and seems not fixed the fault on this PR. I'll try to revert...

@Suzumizaki Suzumizaki closed this Oct 7, 2018

Suzumizaki added some commits Oct 7, 2018

1) Change to use "ManifestFromXMLFile" class instead of Python builti…
…n function "open", because it possibly uses local encodings(not UTF-8).

2) Inside "toprettyxml" function, Using "encoding" parameter when decoding "xmlstr".
3) The test is added. "@skipif_notwin" used, because this test will work only on Windows.
@Suzumizaki

This comment has been minimized.

Copy link
Contributor Author

commented Oct 7, 2018

I did rebase and squash. Thanks.

@Suzumizaki Suzumizaki reopened this Oct 7, 2018

htgoebel added some commits Nov 25, 2018

@htgoebel htgoebel merged commit a0b0152 into pyinstaller:develop Nov 25, 2018

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@htgoebel

This comment has been minimized.

Copy link
Member

commented Nov 25, 2018

Thansk you for this pull-request. I now found time to merge it.

@Suzumizaki

This comment has been minimized.

Copy link
Contributor Author

commented Dec 4, 2018

It looks working fine. Thanks!

@Suzumizaki Suzumizaki deleted the Suzumizaki:fix_reading_msw_manifest_file branch Jul 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.