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

Replace GAP's ExecuteProcess by Julia code #726

Merged
merged 1 commit into from
Apr 21, 2022

Conversation

fingolfin
Copy link
Member

@fingolfin fingolfin commented Oct 7, 2021

This is a step towards getting rid of the GAP SIGCHLD handler

I only did the most basic test (run Exec("ls") from a GAP prompt()), so more tests are absolutely missing, also using this with packages.

On the other hand, things seem to work surprisingly well even without this patch. But SIGCHLD handling in GAP is something we really should try to get rid of (CC @benlorenz)

Resolve #240

@fingolfin fingolfin marked this pull request as draft October 7, 2021 12:23
@benlorenz
Copy link
Member

benlorenz commented Oct 7, 2021

Thanks for working on this, I can try to run some tests with polymake by next week, also with my own patches to polymake removed.

The latest polymake_jll version 400.500.0 also has an updated patch for the sigchild issues which will temporarily set the the SIGCHLD handler to the default while polymake is running subprocesses. I had already patched this for some wrapper compilation issues in a different way but hit such errors again while experimenting with the 4ti2 wrappers. With my new patch all tests look good so far.

@codecov
Copy link

codecov bot commented Apr 14, 2022

Codecov Report

Merging #726 (44d4081) into master (4ab5336) will decrease coverage by 0.02%.
The diff coverage is 75.60%.

❗ Current head 44d4081 differs from pull request most recent head 9af6091. Consider uploading reports for the commit 9af6091 to get more accurate results

@@            Coverage Diff             @@
##           master     #726      +/-   ##
==========================================
- Coverage   78.07%   78.04%   -0.03%     
==========================================
  Files          46       48       +2     
  Lines        3649     3690      +41     
==========================================
+ Hits         2849     2880      +31     
- Misses        800      810      +10     
Impacted Files Coverage Δ
src/exec.jl 70.96% <70.96%> (ø)
src/GAP.jl 82.24% <83.33%> (+0.06%) ⬆️
gap/exec.g 100.00% <100.00%> (ø)

This is a step towards getting rid of the GAP SIGCHLD handler
@fingolfin fingolfin marked this pull request as ready for review April 21, 2022 07:34
@fingolfin fingolfin merged commit 9401650 into oscar-system:master Apr 21, 2022
@fingolfin fingolfin deleted the mh/exec branch April 21, 2022 07:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide replacement for GAP kernel function SyExecuteProcess
2 participants