Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

Test project on Windows with MINGW git (conda2.7&3.4/cpy-3.5) #519

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

Merged
Byron merged 42 commits into gitpython-developers:master from ankostis:appveyor
Oct 1, 2016

Conversation

Copy link
Contributor

@ankostis ankostis commented Sep 25, 2016 •
edited
Loading

  • Use Appveyor integration servers;
  • Copy part of the travis logic, to speed-up test-time (no flake8, site & coverage), the only extra is wheel-packaging;
  • Limit combination of Anaconda, CPython and MINGW/Cygwin in only 6 variations.
  • Replaced poll/select buffer-assembly code with pump-threads (Win TCs faling 90-->20!) and rework unicode handling in many places.
  • Popen() procs it with subprocess.CREATE_NEW_PROCESS_GROUP so that they can be killed.
  • Invoke MINGW git-daemon (instead of git daemon); otherwise it detaches (unix-ism here) from parent git cmd, and then CANNOT DIE!
  • Stop reading directly Popen().proc.stdout/stderr from the launching-thread, it freezes due to GIL/buffering.
    • Unslurp diff consumption code (but not raw-patch) to handle it line-by line to support arbitrary big output.
  • Ensure read-only files do delete on Windows.
    +Rework GitCommandError.str() and add TCs on unicode handling.
  • Fixed TCs on config-reader/writer:
    • Modify lock/read-config-file code to ensure files closed.
    • Use with GitConfigarser() more systematically in TCs.
    • Clean up any locks left hanging from prev TCs.
    • Util: mark lock-files as SHORT_LIVED; save some SSDs...
  • Fixed TestRepo.test_submodule_update():
    • submod: del .git file prior overwrite; Windows denied otherwise!
  • FIX TestRepo.test_untracked_files():
    • In the git add <file> case, PY2 fails when args are unicode.
      Must encode them with locale.getpreferredencoding() AND use SHELL.
  • cmd: add shell into execute() kwds, for overriding USE_SHELL per
    command.
  • repo: replace blocky communicate() in _clone() with thread-pumps (UNNECESSARY, see this later discussion).
  • test_repo.py: unittestize (almost all) assertions.
  • Fixed file-access problems by using with... construct (not complete).
    • Replace open --> with open for index (base and TC).
  • Enabled some TCs, more remain dormant (ie the Hook TC never runs on Linux).
    • test_index.py: Enabled dormant fake-symlink assertion.
  • Various encoding issues.
  • Debug-log all Popen() calls, to collect cmd usage.

Apveyor results

  • Py27: FAILED (SKIP=3, errors=1)
  • Py34: FAILED (SKIP=3, errors=3)
  • Py35 : FAILED (SKIP=3, errors=3)

Byron reacted with heart emoji
Copy link
Member

Byron commented Sep 25, 2016

@ankostis Totally awesome ! Even though I won't be of much help, I do hope that fixing the windows issues work out in the end. That would restore windows platform support and keep it, after all these years of supporting it just on a best-effort basis, which meant not to obviously use non-windows APIs.

@ankostis ankostis force-pushed the appveyor branch 2 times, most recently from 1774f98 to d773d40 Compare September 25, 2016 16:31
Copy link
Contributor Author

Yes, that's a start.
I wish that today this PR become "ready", but currently, it keeps on failing,
and adding gc.collect() in various places...

@ankostis ankostis force-pushed the appveyor branch 3 times, most recently from 5899c59 to d12fdca Compare September 25, 2016 22:01
Copy link
Contributor Author

@Byron the git.testtest_git:TestGit.test_handle_process_output() TC hangs almost on every combination under Windows.
Can you help unstuck it?

ankostis added a commit to ankostis/GitPython that referenced this pull request Sep 25, 2016 
+ Pump once, so when input larger than `mmap.PAGESIZE`, stream is not
emptied.
ankostis added a commit to ankostis/GitPython that referenced this pull request Sep 25, 2016 
+ Pump once, so when input larger than `mmap.PAGESIZE`, stream is not
emptied.
ankostis added a commit to ankostis/GitPython that referenced this pull request Sep 25, 2016 
+ Pump once, so when input larger than `mmap.PAGESIZE`, stream is not
emptied.
ankostis added a commit to ankostis/GitPython that referenced this pull request Sep 26, 2016 
+ Pump code reads only once streams per `_read_lines_from_fno()`
invocation, so when input larger than `mmap.PAGESIZE`, bytes are
forgotten in the stream.
ankostis added a commit to ankostis/GitPython that referenced this pull request Sep 26, 2016 
+ The code in `_read_lines_from_fno()` was reading the stream only once
per invocation, so when input was larger than `mmap.PAGESIZE`, bytes
were forgotten in the stream.
+ Also set deamon-threads.
ankostis added a commit to ankostis/GitPython that referenced this pull request Sep 26, 2016 
+ The code in `_read_lines_from_fno()` was reading the stream only once
per invocation, so when input was larger than `mmap.PAGESIZE`, bytes
were forgotten in the stream.
+ Replaced buffer-banging code with iterate-on-file-descriptors.
+ Also set deamon-threads.
ankostis added a commit to ankostis/GitPython that referenced this pull request Sep 26, 2016 
+ The code in `_read_lines_from_fno()` was reading the stream only once
per invocation, so when input was larger than `mmap.PAGESIZE`, bytes
were forgotten in the stream.
+ Replaced buffer-building code with iterate-on-file-descriptors.
+ Also set deamon-threads.
Copy link
Contributor Author

ankostis commented Sep 26, 2016 •
edited
Loading

@Byron that was a tough one!
The pump code in cmd:handle_process_output() was crafted for the select/poll and was reading the stream only once per _read_lines_from_fno() invocation; so when input was larger than mmap.PAGESIZE, bytes were forgotten in the stream, and the child-process hang.

I replaced the buffer-assembly code with iterate-on-file-descriptors. Actually the code is so simple that it maybe worth to replace the select-poll with the 2-pumps code, for all OSs?

Many (削除) more (削除ここまで) bugs remain in Windows, but no hangs.
[edit]Actually with this bug fixed, the failed TCs on Windows dropped from ~90-->20!!

A side question:
In cmd.handle_process_output() you write:

NOTE: Just joining threads can possibly fail as there is a gap between .start() and when it's
actually started, which could make the wait() call to just return because the thread is not yet
active

Is that possible? I would expect any language allowing such "gap" to be considered outright broken.
But I couldnt google anything relevant?

ankostis added a commit to ankostis/GitPython that referenced this pull request Sep 26, 2016
ankostis added a commit to ankostis/GitPython that referenced this pull request Sep 26, 2016
Copy link
Contributor Author

ankostis commented Oct 1, 2016

Just pushed one last commit with those 3 TCs appropriately skipped.
-- ALL GREEN now --

Especially the one with the incompatible type appears like something that is relatively easy reproduce and fix via smmap.

Actually quite the contrary: I can only reproduce it when i run the full harness; if i launch these 2 TCs alone, they pass! So practically, I cannot enter debug-mode - I have to wait 2' just to reach this point.

PS: You have been invited to become a contributor/maintainer.

Have not received an invitation yet.

Copy link
Member

Byron commented Oct 1, 2016

This is odd:
screen shot 2016年10月01日 at 20 54 24

I have just re-invited you, maybe it comes through. Otherwise you would just have to visit https://github.com/gitpython-developers to see the invitation.

Your last commit was not part of my merge, I will fetch it separately.

Byron pushed a commit that referenced this pull request Oct 1, 2016 
+ Just to see Apveyor all green and merge; the TCs HAVE TO BE FIXED.
Copy link
Member

Byron commented Oct 1, 2016

@ankostis I think it worked ! The last commit has been cherry-picked and AppVeyor is on its way to all-green ! Even though it's not the real thing just yet, I call it an impressive piece of work! Thanks so much !

Right now I wouldn't know how to tackle the smmap issue, just as it's hard to fix anything in a project that is installed as a dependency. Probably it would be easiest if it would be reproducible locally, which I can't even do.
Just so you know, I will probably go to read-only mode for the next days due to my travels, but should be back with replies by the coming weekend.

Copy link
Contributor Author

ankostis commented Oct 1, 2016 •
edited
Loading

@Byron many of the fixes in this PR can be applied also on gitdb, and as you said (haven't checked) on smmp. Is there a chance to tackle these issues in these projects? Or are they "frozen" somehow?

I mean, a lot of problems just go away if resource classes are retrofitted as with .... context resources - but this means API forward compatibility is broken for future clients (existing code should work with the modified libs though).

What do you think, is it worth the effort (but cna't promise anything)?

@ankostis ankostis changed the title (削除) Test project on Windows with MINGW/Cygwin git (conda2.7&3.4/cpy-3.5) (削除ここまで) (追記) Test project on Windows with MINGW git (conda2.7&3.4/cpy-3.5) (追記ここまで) Oct 1, 2016
ankostis added a commit to ankostis/GitPython that referenced this pull request Oct 2, 2016
ankostis added a commit to ankostis/GitPython that referenced this pull request Oct 2, 2016
ankostis added a commit to ankostis/GitPython that referenced this pull request Oct 2, 2016 
...rs#519
+ Regression introduced in d84b960, by a wrong comment
interpretation.
Copy link
Contributor Author

ankostis commented Oct 2, 2016

For reference, reporting the CORRECT Appveyor results after #521 fixes by yarikoptic & ankostis restored skipped TCs:

PY27: 5 errors:
PY34: 4 errors:
PY35: 7 errors:
conda35: 7 errors:

ankostis added a commit to ankostis/GitPython that referenced this pull request Oct 2, 2016 
...preferredencoding()`
+ On Windows, this is the default encoding (usually, the mate-encoding
`mcbs' which points to the current code-page (i.e. `cp1252` for Europe).
@ankostis ankostis modified the milestone: v2.1.0 - proper windows support Oct 15, 2016
Copy link
Member

Byron commented Oct 22, 2016

@ankostis With the latest upgrade of gitdb and smmap to 2.0, it's finally possible again to make new releases. This would allow the changes you talk about to be retro-fitted indeed, and given all the subtle problems GitPython has, it would certainly be worth doing that.

EliahKagan added a commit to EliahKagan/GitPython that referenced this pull request Oct 18, 2023 
In the git.diff.Diff class, the _index_from_patch_format and
_index_from_raw_format methods' docstrings had still described them
as reading from streams, even though they have instead read from
processes (and taken "proc", not "stream", arguments) since gitpython-developers#519
when the change was made in a5db3d3 to fix a freezing bug.
This updates the docstrings to reflect that they read from
processes.
EliahKagan added a commit to EliahKagan/GitPython that referenced this pull request Oct 18, 2023 
In the git.diff.Diff class, the _index_from_patch_format and
_index_from_raw_format methods' docstrings had still described them
as reading from streams, even though they have instead read from
processes (and taken "proc", not "stream", arguments) since gitpython-developers#519
when the change was made in a5db3d3 to fix a freezing bug.
This updates the docstrings to reflect that they read from
processes.
EliahKagan added a commit to EliahKagan/GitPython that referenced this pull request Oct 18, 2023 
In the git.diff.Diff class, the _index_from_patch_format and
_index_from_raw_format methods' docstrings had still described them
as reading from streams, even though they have instead read from
processes (and taken "proc", not "stream", arguments) since gitpython-developers#519
when the change was made in a5db3d3 to fix a freezing bug.
This updates the docstrings to reflect that they read from
processes.
EliahKagan added a commit to EliahKagan/GitPython that referenced this pull request Oct 22, 2023 
In the git.diff.Diff class, the _index_from_patch_format and
_index_from_raw_format methods' docstrings had still described them
as reading from streams, even though they have instead read from
processes (and taken "proc", not "stream", arguments) since gitpython-developers#519
when the change was made in a5db3d3 to fix a freezing bug.
This updates the docstrings to reflect that they read from
processes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Development

Successfully merging this pull request may close these issues.
2 participants

AltStyle によって変換されたページ (->オリジナル) /