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

Improve handling of temp directory in GAP for Windows #3939

Merged
merged 2 commits into from
Apr 9, 2021

Conversation

ChrisJefferson
Copy link
Contributor

@ChrisJefferson ChrisJefferson commented Apr 1, 2020

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 use sh to spawn external programs (sh can still run windows programs)

  1. Don't try to hard-wire the temp directory (and on many computers the guess is wrong). Instead we reconfigure cygwin to tell it to bind /tmp to the correct temp directory (this is done by the installer).

@ChrisJefferson ChrisJefferson added do not merge PRs which are not yet ready to be merged (e.g. submitted for discussion, or test results) release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels Apr 1, 2020
@ChrisJefferson ChrisJefferson changed the title Cygwin fixes for new release scripts Cygwin improvements for new release scripts Apr 1, 2020
Copy link
Member

@fingolfin fingolfin left a 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?

@fingolfin
Copy link
Member

fingolfin commented Apr 15, 2020

Also, could you explain in how far this breaks the current release if used in a release made using the current process for making a Windows release?

@fingolfin fingolfin added the os: windows Issues and PRs that are (at least partially) specific to Windows label Apr 15, 2020
@ChrisJefferson
Copy link
Contributor Author

ChrisJefferson commented Apr 15, 2020

So, the changes are:

1) Use the program cygstart. We don't currently package that, and if we did we wouldn't set up the paths correctly.

2) Similarly, run external commands via sh, we don't distribute that either.

  1. Use /tmp for temp files -- at the moment that won't work because we don't set up standard unix directories. Further, in the new installer we make cygwin point /tmp at the windows temp directory.

@codecov
Copy link

codecov bot commented Feb 18, 2021

Codecov Report

Merging #3939 (f8ece80) into master (ce4b50f) will increase coverage by 10.22%.
The diff coverage is 0.00%.

@@             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     
Impacted Files Coverage Δ
lib/helpview.gi 37.24% <0.00%> (-0.69%) ⬇️
lib/process.gi 27.20% <0.00%> (-42.80%) ⬇️
src/streams.c 76.93% <ø> (-5.17%) ⬇️
lib/helpt2t.gi 0.23% <0.00%> (-83.14%) ⬇️
lib/autsr.gi 1.09% <0.00%> (-66.09%) ⬇️
lib/csetpc.gi 39.42% <0.00%> (-48.58%) ⬇️
src/vec8bit.h 51.78% <0.00%> (-42.86%) ⬇️
lib/gpfpiso.gi 38.48% <0.00%> (-37.26%) ⬇️
lib/permdeco.gi 37.93% <0.00%> (-34.65%) ⬇️
... and 216 more

@ChrisJefferson ChrisJefferson force-pushed the cygwin-fixes branch 6 times, most recently from b57190d to 25cfdd0 Compare February 23, 2021 18:15
@ChrisJefferson ChrisJefferson removed the do not merge PRs which are not yet ready to be merged (e.g. submitted for discussion, or test results) label Apr 2, 2021
@ChrisJefferson
Copy link
Contributor Author

ChrisJefferson commented Apr 2, 2021

Hi @fingolfin or @wilfwilson , this very old PR is now ready to merge, it will improve future cygwin releases.

Copy link
Member

@wilfwilson wilfwilson left a 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.

@wilfwilson
Copy link
Member

In more detail, @ChrisJefferson said this PR doesn't break the existing GAP release system for Windows. It also does two things:

  • The changes to the temporary directory that Chris mentioned above
  • This redefines ARCH_IS_WINDOWS to mean 'is running in Cygwin but not in a full Cygwin installation' (see the changed documentation).

@wilfwilson wilfwilson merged commit f541fa8 into gap-system:master Apr 9, 2021
@ChrisJefferson ChrisJefferson deleted the cygwin-fixes branch April 1, 2022 08:17
@fingolfin fingolfin changed the title Cygwin improvements for new release scripts Improve GAP for Windows (using Cygwin) Aug 17, 2022
@fingolfin fingolfin added release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes and removed release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels Aug 17, 2022
@fingolfin fingolfin changed the title Improve GAP for Windows (using Cygwin) Improve handling of temp directory in GAP for Windows (using Cygwin) Aug 17, 2022
@fingolfin fingolfin changed the title Improve handling of temp directory in GAP for Windows (using Cygwin) Improve handling of temp directory in GAP for Windows Aug 17, 2022
@fingolfin fingolfin added the kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements label Aug 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements os: windows Issues and PRs that are (at least partially) specific to Windows release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants