Compatibility with GAP.jl #3497

sebasguts · 2019-06-12T20:49:04Z

Description

Added installation of GAPTypes.jl to configure when compilation
with Julia is requested
Made MPtr, Bag, and LargeBag Julia types instances of GapObj

Please use the following template to submit a pull request, filling
in at least the "Text for release notes" and/or "Further details".
Thank You!

Checklist for pull request reviewers

proper formatting

If your code contains kernel C code, run clang-format on it; the
simplest way is to use git clang-format, e.g. like this (don't
forget to commit the resulting changes):

git clang-format $(git merge-base master)

usage of relevant labels
1. either release notes: not needed or release notes: to be added
2. at least one of the labels bug or enhancement or new feature
3. for changes meant to be backported to stable-4.X add the backport-to-4.X label
4. consider adding any of the labels build system, documentation, kernel, library, tests
runnable tests
adequate pull request title
well formulated text for release notes
relevant documentation updates
sensible comments in the code

codecov · 2019-06-12T22:14:18Z

Codecov Report

Merging #3497 into master will increase coverage by 0.68%.
The diff coverage is 78.57%.

@@            Coverage Diff             @@
##           master    #3497      +/-   ##
==========================================
+ Coverage   84.75%   85.43%   +0.68%     
==========================================
  Files         698      699       +1     
  Lines      346577   346778     +201     
==========================================
+ Hits       293728   296273    +2545     
+ Misses      52849    50505    -2344

Impacted Files	Coverage Δ
src/julia_gc.c	`82.72% <78.57%> (+5.32%)`	⬆️
src/gasman.c	`86.28% <0%> (-3.66%)`	⬇️
src/gap.c	`81.18% <0%> (-0.37%)`	⬇️
lib/debug.g	`23.57% <0%> (ø)`
lib/algrep.gi	`80.6% <0%> (+0.04%)`	⬆️
lib/cyclotom.gi	`80.9% <0%> (+0.08%)`	⬆️
lib/ratfun.gi	`91.19% <0%> (+0.08%)`	⬆️
src/plist.c	`95.06% <0%> (+0.08%)`	⬆️
lib/meataxe.gi	`87.54% <0%> (+0.08%)`	⬆️
lib/oprtperm.gi	`93.75% <0%> (+0.1%)`	⬆️
... and 119 more

src/julia_gc.c

configure.ac

src/julia_gc.c

sebasguts · 2019-06-17T06:58:28Z

I added error checking, and also updated this PR since GAPTypes is a released package now. If tests pass, I think this could be merged.

coveralls · 2019-06-17T08:03:03Z

Coverage remained the same at 85.26% when pulling 145b186 on sebasguts:gap-jl-gaptypes into ce16a7d on gap-system:master.

olexandr-konovalov

@sebasguts since you say that this PR is important "Because we need it to use GAP.jl properly in other Julia modules. Plus, we could then finally pin a released GAP version in GAP.jl, no downloading master anymore", please mention this in release notes under

gap/doc/changes/changes-4.10.xml

Line 832 in ce16a7d

Fix stack scanning for the Julia GC when GAP is used as a library

fingolfin · 2019-06-17T12:06:45Z

src/julia_gc.c

@@ -817,6 +817,26 @@ void InitBags(UInt initial_size, Bag * stack_bottom, UInt stack_align)
    max_pool_obj_size = jl_gc_max_internal_obj_size();
    jl_gc_enable_conservative_gc_support();
    jl_init();
+
+    // Import GAPTypes module to have access
+    // to GapObj abstract type.


Weird wrapping, perhaps that comment could go into a single line? (Clearly, this is very important ;-)

fingolfin

LGTM, except for lack of testing what happens if GAPTypes.jl is missing (e.g. because the user wiped ~/.julia, which I sometimes do, so this is actually quite realistic).

sebasguts · 2019-06-17T12:41:52Z

@fingolfin @alex-konovalov I think I adressed both comments now.

olexandr-konovalov · 2019-06-17T13:38:48Z

doc/changes/changes-4.10.xml

@@ -667,6 +667,9 @@ Specify the Julia binary instead of the Julia prefix
 Export Julia <C>CFLAGS</C>, <C>LDFLAGS</C>, and <C>LIBS</C> to <C>sysinfo.gap</C>
 (<URL><LinkText>&Hash;3248</LinkText><Link>https://github.com/gap-system/gap/pull/3248</Link></URL>).
 </Item>
+<Item>
+Make <C>MPtr</C> Julia type of GAP objects depend on abstract Julia <C>GapObj</C> type


Please use &GAP; for GAP

olexandr-konovalov

Fine w/me, thanks

doc/changes/changes-4.10.xml

fingolfin · 2019-06-17T15:37:42Z

configure.ac

@@ -497,6 +497,9 @@ AS_IF([test "x$with_julia" != xno ],[
  AS_IF([ test $? != 0 ], [AC_MSG_ERROR([failed to obtain JULIA_LIBS from julia-config.jl])])
  JULIA_LIBS=${JULIA_LIBS//\'/}
  AC_MSG_RESULT([${JULIA_LIBS}])
+  AC_MSG_NOTICE([installing GAPTypes.jl])
+  ${JULIA} -e 'import Pkg; Pkg.add("GAPTypes")'
+  AS_IF([ test $? != 0 ], [AC_MSG_ERROR([failed to install GAPTypes.jl])])


Did you verify that this works as intended? (E.g. by changing the previous line to use Pkg.add("foobarquux") to force an error?

fingolfin · 2019-06-17T15:39:14Z

doc/changes/changes-4.10.xml

@@ -667,6 +667,10 @@ Specify the Julia binary instead of the Julia prefix
 Export Julia <C>CFLAGS</C>, <C>LDFLAGS</C>, and <C>LIBS</C> to <C>sysinfo.gap</C>
 (<URL><LinkText>&Hash;3248</LinkText><Link>https://github.com/gap-system/gap/pull/3248</Link></URL>).
 </Item>
+<Item>
+Make <C>MPtr</C> Julia type of &GAP; objects depend on abstract Julia <C>GapObj</C> type


"Depends" sounds wrong to me. "Derive from" ? "Be a subtype of"

Suggested change

Make <C>MPtr</C> Julia type of &GAP; objects depend on abstract Julia <C>GapObj</C> type

Change <C>MPtr</C> Julia type of &GAP; objects to be a subtype of the abstract Julia <C>GapObj</C> type

doc/changes/changes-4.10.xml

fingolfin · 2019-06-17T15:39:55Z

doc/changes/changes-4.10.xml

@@ -667,6 +667,10 @@ Specify the Julia binary instead of the Julia prefix
 Export Julia <C>CFLAGS</C>, <C>LDFLAGS</C>, and <C>LIBS</C> to <C>sysinfo.gap</C>
 (<URL><LinkText>&Hash;3248</LinkText><Link>https://github.com/gap-system/gap/pull/3248</Link></URL>).
 </Item>
+<Item>
+Make <C>MPtr</C> Julia type of &GAP; objects depend on abstract Julia <C>GapObj</C> type


"Depends" sounds wrong to me. "Derive from" ? "Be a subtype of"

Suggested change

Make <C>MPtr</C> Julia type of &GAP; objects depend on abstract Julia <C>GapObj</C> type

Change <C>MPtr</C> Julia type of &GAP; objects to be a subtype of the abstract Julia <C>GapObj</C> type

provided by the Julia package <C>GAPTypes.jl</C>

* Added installation of GAPTypes.jl to configure when compilation with Julia is requested * Made MPtr, Bag, and LargeBag Julia types instances of GapObj

sebasguts · 2019-06-17T15:52:33Z

Addressed the last comments. Feel free to merge it.

fingolfin · 2019-06-19T14:07:30Z

Backported in commit df86a36

fingolfin · 2019-08-22T08:30:08Z

We forgot to add this to the release notes for 4.10.2, but it's not so important, so I decided to omit this from the 4.11 release notes, too.

sebasguts added release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes topic: julia Julia GC integration and related matters labels Jun 12, 2019

sebasguts requested review from fingolfin and rbehrends June 12, 2019 20:49

sebasguts force-pushed the gap-jl-gaptypes branch from f05f400 to 968e614 Compare June 12, 2019 21:09

fingolfin reviewed Jun 13, 2019

View reviewed changes

src/julia_gc.c Show resolved Hide resolved

configure.ac Outdated Show resolved Hide resolved

src/julia_gc.c Show resolved Hide resolved

src/julia_gc.c Outdated Show resolved Hide resolved

sebasguts force-pushed the gap-jl-gaptypes branch 2 times, most recently from 245cfac to 1d83398 Compare June 17, 2019 06:57

olexandr-konovalov added this to the GAP 4.10.2 milestone Jun 17, 2019

olexandr-konovalov added backport-to-4.10 release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes and removed release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes labels Jun 17, 2019

olexandr-konovalov requested changes Jun 17, 2019

View reviewed changes

fingolfin reviewed Jun 17, 2019

View reviewed changes

fingolfin approved these changes Jun 17, 2019

View reviewed changes

sebasguts force-pushed the gap-jl-gaptypes branch 2 times, most recently from 23581b0 to af4e2aa Compare June 17, 2019 12:41

olexandr-konovalov reviewed Jun 17, 2019

View reviewed changes

sebasguts force-pushed the gap-jl-gaptypes branch from af4e2aa to 0201022 Compare June 17, 2019 13:56

olexandr-konovalov approved these changes Jun 17, 2019

View reviewed changes

olexandr-konovalov requested changes Jun 17, 2019

View reviewed changes

doc/changes/changes-4.10.xml Show resolved Hide resolved

sebasguts force-pushed the gap-jl-gaptypes branch from 0201022 to 44e5337 Compare June 17, 2019 14:18

fingolfin reviewed Jun 17, 2019

View reviewed changes

fingolfin approved these changes Jun 17, 2019

View reviewed changes

olexandr-konovalov approved these changes Jun 17, 2019

View reviewed changes

Compatibility with GAP.jl

145b186

* Added installation of GAPTypes.jl to configure when compilation with Julia is requested * Made MPtr, Bag, and LargeBag Julia types instances of GapObj

sebasguts force-pushed the gap-jl-gaptypes branch from 2c1f40d to 145b186 Compare June 17, 2019 15:52

fingolfin merged commit 69c07d1 into gap-system:master Jun 17, 2019

fingolfin added backport-to-4.10-DONE and removed backport-to-4.10 labels Jun 19, 2019

fingolfin added release notes: added PRs introducing changes that have since been mentioned in the release notes and removed release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels Aug 22, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Compatibility with GAP.jl #3497

Compatibility with GAP.jl #3497

sebasguts commented Jun 12, 2019

codecov bot commented Jun 12, 2019 •

edited

Loading

sebasguts commented Jun 17, 2019

coveralls commented Jun 17, 2019 •

edited

Loading

olexandr-konovalov left a comment •

edited

Loading

fingolfin Jun 17, 2019

fingolfin left a comment

sebasguts commented Jun 17, 2019

olexandr-konovalov Jun 17, 2019

sebasguts Jun 17, 2019

olexandr-konovalov left a comment

fingolfin Jun 17, 2019

sebasguts Jun 17, 2019

fingolfin Jun 17, 2019

fingolfin Jun 17, 2019

sebasguts commented Jun 17, 2019

fingolfin commented Jun 19, 2019 •

edited by olexandr-konovalov

Loading

fingolfin commented Aug 22, 2019

	Make <C>MPtr</C> Julia type of &GAP; objects depend on abstract Julia <C>GapObj</C> type
	Change <C>MPtr</C> Julia type of &GAP; objects to be a subtype of the abstract Julia <C>GapObj</C> type

	Make <C>MPtr</C> Julia type of &GAP; objects depend on abstract Julia <C>GapObj</C> type
	Change <C>MPtr</C> Julia type of &GAP; objects to be a subtype of the abstract Julia <C>GapObj</C> type
	provided by the Julia package <C>GAPTypes.jl</C>

Compatibility with GAP.jl #3497

Compatibility with GAP.jl #3497

Conversation

sebasguts commented Jun 12, 2019

Description

Checklist for pull request reviewers

codecov bot commented Jun 12, 2019 • edited Loading

Codecov Report

sebasguts commented Jun 17, 2019

coveralls commented Jun 17, 2019 • edited Loading

olexandr-konovalov left a comment • edited Loading

Choose a reason for hiding this comment

fingolfin Jun 17, 2019

Choose a reason for hiding this comment

fingolfin left a comment

Choose a reason for hiding this comment

sebasguts commented Jun 17, 2019

olexandr-konovalov Jun 17, 2019

Choose a reason for hiding this comment

sebasguts Jun 17, 2019

Choose a reason for hiding this comment

olexandr-konovalov left a comment

Choose a reason for hiding this comment

fingolfin Jun 17, 2019

Choose a reason for hiding this comment

sebasguts Jun 17, 2019

Choose a reason for hiding this comment

fingolfin Jun 17, 2019

Choose a reason for hiding this comment

fingolfin Jun 17, 2019

Choose a reason for hiding this comment

sebasguts commented Jun 17, 2019

fingolfin commented Jun 19, 2019 • edited by olexandr-konovalov Loading

fingolfin commented Aug 22, 2019

codecov bot commented Jun 12, 2019 •

edited

Loading

coveralls commented Jun 17, 2019 •

edited

Loading

olexandr-konovalov left a comment •

edited

Loading

fingolfin commented Jun 19, 2019 •

edited by olexandr-konovalov

Loading