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

Get rid of GAPTypes.jl #322

Merged
merged 1 commit into from
Jan 27, 2020
Merged

Conversation

fingolfin
Copy link
Member

@fingolfin fingolfin commented Jan 23, 2020

This requires a custom GAP branch for now, but see also gap-system/gap#3867

Fixes #305
Fixes #310

@codecov
Copy link

codecov bot commented Jan 23, 2020

Codecov Report

Merging #322 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #322   +/-   ##
=======================================
  Coverage   76.58%   76.58%           
=======================================
  Files          62       62           
  Lines        4428     4428           
=======================================
  Hits         3391     3391           
  Misses       1037     1037
Impacted Files Coverage Δ
src/GAP.jl 92.85% <ø> (ø) ⬆️
deps/build.jl 0% <ø> (ø) ⬆️

@fingolfin fingolfin force-pushed the mh/GAPTypes branch 4 times, most recently from ca9b26b to c13b863 Compare January 23, 2020 23:27
@fingolfin fingolfin changed the title WIP: get rid of GAPTypes.jl Get rid of GAPTypes.jl Jan 23, 2020
@fingolfin
Copy link
Member Author

Unfortunately the new GAP.jl in this PR requires the new GAP; mixing either of them with an old version of their counterpart doesn't work (some cases work, but not the general thing).
I see two options to go forward:

  1. We just accept that, and try to merge both changes about the same time, and then immediately make a new GAP.jl release so as to not annoy users too much
  2. I try to come up with a way to split this into separate updates which always retain compatibility to one version prior:
  • this PR would get split into at least two parts: one adds the call to GAP_register_GapObj but basically nothing else (in particular, doesn't drop GAPTypes)
  • we make a release of GAP.jl immediately afterwards
  • next, GAP is update by merging julia: remove the need for GAPTypes.jl gap-system/gap#3867 into master and stable-4.11; this will work with the latest released GAP.jl; it drops use of GAPTypes from GAP
  • finally, we update GAP.jl once more, removing references to GAPTypes here, too.

The second approach is more work but should be safer, and requires less perfect timing. So I'll try and see if I can do it this way.

@fingolfin fingolfin merged commit f2c68b4 into oscar-system:master Jan 27, 2020
@fingolfin fingolfin deleted the mh/GAPTypes branch January 27, 2020 22:32
@kalmarek
Copy link

I'd go for the first version, it's less work and GAP.jl is still living on the bleeding edge ;-)

@fingolfin
Copy link
Member Author

@kalmarek not sure what you refer to with "first version"? Anyway, this PR is already merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants