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 failure loading a workspace created while GAP's stdin was a file into a GAP with stdin attached to a terminal #5573

Merged
merged 2 commits into from
Jan 12, 2024

Conversation

fingolfin
Copy link
Member

... by reverting a previous change that made us disable readline when stdin is not connected to a terminal (see #4495). While that still seems useful, and avoids certain issues when piping GAP output into files, it needs a better implication that avoids the issues described in #5014

Also add a separate CI test suite target "testworkspace" for testing for this issue, and potentially other workspace related issues.

Resolves #5405
Resolves #5014

CC @zickgraf

Copy link
Contributor

@ChrisJefferson ChrisJefferson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree this puts us in a better situation

... by reverting a previous change that made us disable readline
when stdin is not connected to a terminal (see
gap-system#4495). While that still
seems useful, and avoids certain issues when piping GAP output
into files, it needs a better implication that avoids the issues
described in gap-system#5014

Also add a separate CI test suite target "testworkspace" for testing
for this issue, and potentially other workspace related issues.
@zickgraf
Copy link
Contributor

Thank you @fingolfin! We are not using workspaces anymore on plesken, so I cannot easily test if this resolves #5014 there. But it looks like the situation is covered by the new tests you added.

@fingolfin
Copy link
Member Author

To be clear, without the fix part in this part, the new testworkspace fails in just the way described in #5014 (and for the exact reasons identified there). I.e. I am confident the fix / workaround resolve that issue.

Unfortunately, it seems that the tests uncovered a new unrelated problem, which causes tst/testinstall/kernel/opers.tst to fail. I'll take a look

@fingolfin
Copy link
Member Author

opers.tst failure should be fixed now

@fingolfin fingolfin merged commit 588eff9 into gap-system:master Jan 12, 2024
22 checks passed
@fingolfin fingolfin deleted the mh/fix-workspace branch January 12, 2024 12:17
@fingolfin fingolfin added kind: bug Issues describing general bugs, and PRs fixing them kind: bug: unexpected error Issues describing bugs in which computation unexpectedly encounters an error, and PRs fixing them labels Jan 22, 2024
@fingolfin fingolfin changed the title Fix readline related workspace issue Fix failure loading a workspace created while GAP's stdin was a file into a GAP with stdin attached to a terminal Jan 22, 2024
@fingolfin fingolfin added the release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes label Jan 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: bug: unexpected error Issues describing bugs in which computation unexpectedly encounters an error, and PRs fixing them kind: bug Issues describing general bugs, and PRs fixing them release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes
Projects
None yet
3 participants