Skip to content

Conversation

fingolfin
Copy link
Contributor

@fingolfin fingolfin commented May 31, 2023

Addresses parts of #35361

@fchapoton
Copy link
Contributor

This fails on garbage collection.

@github-actions
Copy link

github-actions bot commented Jun 4, 2023

Documentation preview for this PR (built with commit cff047d) is ready! 🎉

Copy link
Contributor

@fchapoton fchapoton left a comment

Choose a reason for hiding this comment

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

ok, thanks

@fingolfin
Copy link
Contributor Author

There are all kinds of Conda errors now, but I think / hope they are not caused by this PR?

@fchapoton
Copy link
Contributor

I have no idea. They seem rather to be of the standard "dependencies broken" style in the conda building.

@vbraun vbraun merged commit d7bb52a into sagemath:develop Jun 11, 2023
@fingolfin fingolfin deleted the mh/more-libgap branch June 13, 2023 10:30
vbraun pushed a commit that referenced this pull request Jun 21, 2023
gh-35735: gap: use CALL_WITH_STREAM to redirect output to string
    
<!-- Please provide a concise, informative and self-explanatory title.
-->
<!-- Don't put issue numbers in the title. Put it in the Description
below. -->
<!-- For example, instead of "Fixes #12345", use "Add a new method to
multiply two integers" -->

### 📚 Description

This avoids use of deeply internal GAP APIs which may change at any
time. In particular `TypOutputFile` is likely to change again and again.

In contrast, `CALL_WITH_STREAM` is on a much higher level (and even
reachable from the GAP language itself). We have no plans to change it.

Together with PR #35702 this resolves #35361.

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. It should be `[x]` not `[x
]`. -->

- [ ] The title is concise, informative, and self-explanatory.
- [ ] The description explains in detail what this PR is about.
- [ ] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation accordingly.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on
- #12345: short description why this is a dependency
- #34567: ...
-->

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: #35735
Reported by: Max Horn
Reviewer(s): Frédéric Chapoton
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants