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 calling AtExit methods after loading a workspace #2578

Merged
merged 2 commits into from
Jun 28, 2018

Conversation

ChrisJefferson
Copy link
Contributor

@ChrisJefferson ChrisJefferson commented Jun 26, 2018

This patch just ensures we call "CLEAN_UP_PROGRAM" after restoring a workspace, mainly so the AtExit handlers are run.

While fixing this I noticed a different (small) bug, which is when restoring a workspace some handlers end up getting installed twice. The easiest fix for this (in my opinion) is to just skip the second time the handler is installed.

Fixes #2558

@ChrisJefferson
Copy link
Contributor Author

Fixes #2558. This is unfortunately hard to test automatically, so I'd appresiate someone sanity checking it works. Basically, you should find saving history, and clean up of TemporaryDirectory, didn't work before this patch and does after this patch, if GAP is loaded with -L.

@ChrisJefferson
Copy link
Contributor Author

Fixes #2558. This is unfortunately hard to test automatically, so I'd appresiate someone sanity checking it works. Basically, you should find saving history didn't work before this patch and does after this patch, if GAP is started from a workspace.

@codecov
Copy link

codecov bot commented Jun 26, 2018

Codecov Report

Merging #2578 into master will increase coverage by <.01%.
The diff coverage is 83.33%.

@@            Coverage Diff             @@
##           master    #2578      +/-   ##
==========================================
+ Coverage    74.6%   74.61%   +<.01%     
==========================================
  Files         481      481              
  Lines      242386   242392       +6     
==========================================
+ Hits       180843   180862      +19     
+ Misses      61543    61530      -13
Impacted Files Coverage Δ
lib/session.g 56.86% <100%> (+0.86%) ⬆️
lib/oper.g 80.56% <80%> (-0.01%) ⬇️
src/iostream.c 62.64% <0%> (-1.14%) ⬇️
hpcgap/lib/hpc/stdtasks.g 64.19% <0%> (-0.52%) ⬇️
lib/files.gd 63.54% <0%> (+1.04%) ⬆️
lib/files.gi 52.04% <0%> (+7.37%) ⬆️

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 fixes a regression I introduced when merging HPC-GAP, see e322531 - thanks for that!

The first commit could also be backported to GAP 4.9, As it fixes a regression compared to 4.8. (The second one could also be backported IMO)

But please add labels.

@ChrisJefferson ChrisJefferson added kind: bug Issues describing general bugs, and PRs fixing them backport-to-4.9 labels Jun 26, 2018
@fingolfin
Copy link
Member

@mohamed-barakat please test if this solves your problem

@olexandr-konovalov
Copy link
Member

I've assigned this to 4.9.2 milestone - in a hope that @mohamed-barakat may confirm it quickly. Otherwise may be delayed till 4.9.3.

@mohamed-barakat
Copy link
Member

I am happy, thanks.

@fingolfin fingolfin merged commit 1058408 into gap-system:master Jun 28, 2018
@olexandr-konovalov olexandr-konovalov added backport-to-4.9 release notes: added PRs introducing changes that have since been mentioned in the release notes and removed backport-to-4.9 labels Jun 28, 2018
@ChrisJefferson ChrisJefferson deleted the fix-restore branch July 9, 2018 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: bug Issues describing general bugs, and PRs fixing them release notes: added PRs introducing changes that have since been mentioned in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

History does not work anymore if work space was loaded
4 participants