-
Notifications
You must be signed in to change notification settings - Fork 160
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
Improve handling of temp directory in GAP for Windows #3939
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is marked as "do no merge", and I also cannot test it, but overall it looks sensible to me. I could approve it if required, but I'd feel more comfortable if somebody who actually can test it (e.g. @PaulaHaehndel ) were to test and review it; but as long as it is marked "do not merge", I am not sure that's sensible?
Also, could you explain in how far this breaks |
So, the changes are:
|
db60d48
to
c2e1e80
Compare
c2e1e80
to
498a24c
Compare
Codecov Report
@@ Coverage Diff @@
## master #3939 +/- ##
===========================================
+ Coverage 82.26% 92.48% +10.22%
===========================================
Files 686 714 +28
Lines 296672 790718 +494046
===========================================
+ Hits 244045 731321 +487276
- Misses 52627 59397 +6770
|
b57190d
to
25cfdd0
Compare
25cfdd0
to
5d952f5
Compare
Hi @fingolfin or @wilfwilson , this very old PR is now ready to merge, it will improve future cygwin releases. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ChrisJefferson and I have just been talking about it this PR, and associated Cygwin-related stuff. I've crossed out some text in the previous parts of this discussion, because they're no longer applicable.
Given our discussion, I think this should be merged.
In more detail, @ChrisJefferson said this PR doesn't break the existing GAP release system for Windows. It also does two things:
|
This improves the Cygwin release in a few ways.
This should only be merged once we have decided we are definitely moving to my new Windows release, because this breaks the current release.The improvements are:
1) Use 'cygstart' to open webpages. This deals with unix vs windows filenames. It doesn't let us handle anchors, but after some googling this appears impossible in windows, as#
can appear in filenames.2) always usesh
to spawn external programs (sh
can still run windows programs)/tmp
to the correct temp directory (this is done by the installer).