Remove DEBUG_LOADING, InfoRead1, and InfoRead2 #2237

markuspf · 2018-03-04T00:19:09Z

They are a leftover of a legacy debugging mechanism. InfoRead1 and InfoRead2 are moved into obsoletes.gd.

Unfortunately the following packages still refer to InfoRead1 or InfoRead2:

atlasrep (uses InfoRead1 for info output, should be replaced by Info())
gbnp (only saves and restores state in tests)
guava (only saves and restores state) done in Remove hack around InfoRead1 and InfoRead2 gap-packages/guava#33
hecke (spurious misuse of InfoRead1): TODO Remove use of InfoRead1 gap-packages/hecke#5
xgap: TODO Remove use of InfoRead1 gap-packages/xgap#7

These packages will break with this PR if GAP is started without obsoletes.

codecov · 2018-03-04T00:42:29Z

Codecov Report

Merging #2237 into master will decrease coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2237      +/-   ##
==========================================
- Coverage   85.13%   85.12%   -0.01%     
==========================================
  Files         110      110              
  Lines       56964    56960       -4     
==========================================
- Hits        48494    48489       -5     
- Misses       8470     8471       +1

Impacted Files	Coverage Δ
src/gap.c	`83.81% <ø> (-0.1%)`	⬇️
src/stats.c	`95.07% <0%> (-0.21%)`	⬇️

fingolfin

Looks good in general to me, but of course it breaks packages using InfoRead1 and InfoRead2, as you pointed out yourself. But there's a quick solution to that, too: We can just defined them in lib/obsolete.{gi,gd} for now, with a list of packages still using them (as we do with lots of other functions).

Although this would break the "Toggle Library Read Mesg" menu entry" in xgap when used together with the -D option, but I think that's not something to worry about (I guess it could be changed to just toggle GAPInfo.CommandLineOptions.D).

lib/files.gi

fingolfin · 2018-03-04T11:46:05Z

lib/init.g

-    InfoRead1( "#I  reading ", name, "\n" );
+    if GAPInfo.CommandLineOptions.D then
+        Print( "#I  reading ", name, "\n" );
+    fi;


I briefly thought "hmm, why not introduce an InfoRead info class, and use that here, as Info(InfoRead, 1, "reading ", name"); but I suppose the answer for that is that at this point, we are still far away from loading lib/info.gi, so we can't really use Info yet. OK.

I tried introducing an InfoClass for this purpose, but as you say the infrastructure is not initialised when we'd want to use it.

One could (independently of this PR) try moving the info infrastructure as early as possible in startup.

I think it is fine to leave this with a Print statement, but perhaps add a comment like

We cannot use Info yet, because info.gi has not yet been read

lib/files.gi

fingolfin · 2018-03-04T13:09:12Z

hecke, guava and xgap are on GitHub, so we can send pull requests. For atlasrep, we need to talk to @ThomasBreuer -- it seems indent logical to use Info there, but then an info class is needed. Perhaps we should define an InfoRead class after all, and set it based on the initial value of GAPInfo.CommandLineOptions.D (we still couldn't use it in the early library initialization; also, it should probably be reset in a PostRestore method)

This leaves gbnp, which "only" uses this in test/testall.g -- but note: This is not the TestFile in PackageInfo.g -- that's test/test-all.tst. Nothing seems to reference test/testall.g, so I think that file can just be removed; and we don't need to worry about breaking gbnp with this change (though we should still notify its maintainers about this)

markuspf · 2018-03-04T17:14:53Z

Re-added InfoRead1 and InfoRead2 to obsoletes.gd to prevent packages from breaking should we decide to merge this PR.

ThomasBreuer · 2018-03-05T13:16:33Z

Concerning the use of InfoRead1 in the AtlasRep package,
I will replace this by a mechanism of its own inside the package.
From this point of view, there is no need to introduce a new Info class.

fingolfin · 2018-03-10T17:07:04Z

lib/init.g

-    InfoRead1( "#I  reading ", name, "\n" );
+    if GAPInfo.CommandLineOptions.D then
+        Print( "#I  reading ", name, "\n" );
+    fi;


I think it is fine to leave this with a Print statement, but perhaps add a comment like

We cannot use Info yet, because info.gi has not yet been read

lib/files.gd

fingolfin · 2018-03-20T12:15:05Z

I anybody working on PRs for the affected packages, resp. emailing the GBNP maintainer?

markuspf · 2018-06-06T08:42:05Z

I will email the GBNP people. I think it's best to then remove InfoRead as its not used anymore.

fingolfin · 2018-06-07T11:12:51Z

@markuspf did you already do that? One of the authors, Arjeh Cohen, is retired; the other is Jan-Willem Knopper, and I am not sure how interested he is in maintaining GBNP he is. Perhaps we can convince them to let us maintain GBNP for them, and move it to GitHub etc. I know both authors, as well as GBNP, and could also talk to them.

fingolfin · 2018-06-10T23:07:49Z

Just for reference, I emailed Arjeh and Jan Willem, and JW replied and promised to work on an update.

While I approve in general, clearly we need to wait till packages are adjusted. Also, this PR is not mergeable right now.

olexandr-konovalov · 2018-09-29T23:20:52Z

I cross-referenced this in #2804 under "PRs blocked by packages".

fingolfin

Actually, is there any reason not to merge this? Sure, a few packages are broken if GAP is started w/o obsoletes -- but that's already now the case, and in a sense, the very reason we have an "obsoletes" mechanism, no?
Of course we should still keep an open issue tracking the status of this in various packages.

Thoughts? Of course it would be best to rebase this PR one more time, to make sure it's still working fine with latest master.

markuspf · 2018-11-10T17:01:55Z

Rebased, waiting for AppVeyor to get its act together.

I agree this should then be merged.

olexandr-konovalov · 2018-11-10T18:41:19Z

lib/obsolete.gd

+##  InfoRead used to be used to print when a file is read using `Read()`
+##  
+##  These variables are still referred to by atlasrep, gbnp, guava, hecke
+##  and xgap.


@markuspf please update this comment - only atlasrep and gbnp remain (hecke and xgap were done today by @fingolfin).

@markuspf I see that the last sentence in this comment has been deleted completely - why? It should say These variables are still referred to by atlasrep, gbnp. @fingolfin spent some time on updating these comments recently, and made sure that they provide the actual list of packages that still use obsoletes.

markuspf added the do not merge PRs which are not yet ready to be merged (e.g. submitted for discussion, or test results) label Mar 4, 2018

markuspf force-pushed the remove-info-read branch from 2002950 to 5d6828f Compare March 4, 2018 00:57

fingolfin reviewed Mar 4, 2018

View reviewed changes

markuspf force-pushed the remove-info-read branch from 5d6828f to dfacaeb Compare March 4, 2018 17:03

markuspf force-pushed the remove-info-read branch 2 times, most recently from ae9bbb5 to 9600511 Compare March 4, 2018 22:46

fingolfin previously approved these changes Mar 10, 2018

View reviewed changes

markuspf force-pushed the remove-info-read branch 2 times, most recently from 25fb1f3 to 47f9551 Compare June 18, 2018 14:09

olexandr-konovalov mentioned this pull request Sep 29, 2018

Packages: monitoring readiness for GAP 4.10 #2804

Closed

50 tasks

This was referenced Oct 9, 2018

Remove use of InfoRead1 gap-packages/hecke#5

Closed

Remove use of InfoRead1 gap-packages/xgap#7

Closed

Remove use of InfoRead1 gap-packages/gbnp#3

Closed

olexandr-konovalov mentioned this pull request Nov 9, 2018

Packages: monitoring readiness for GAP 4.11 #2984

Closed

31 tasks

fingolfin approved these changes Nov 10, 2018

View reviewed changes

Markus Pfeiffer added 2 commits November 10, 2018 15:43

Remove DEBUG_LOADING, InfoRead1, and InfoRead2

b3317e8

Simplify Read() a bit

043d4e5

markuspf force-pushed the remove-info-read branch from 47f9551 to 85d923c Compare November 10, 2018 15:47

olexandr-konovalov reviewed Nov 10, 2018

View reviewed changes

olexandr-konovalov mentioned this pull request Nov 13, 2018

Diffs in AtlasRep standard tests #1617

Closed

markuspf added release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes and removed do not merge PRs which are not yet ready to be merged (e.g. submitted for discussion, or test results) labels Nov 14, 2018

Add InfoRead1 and InfoRead2 to obsoletes

1751ddb

markuspf force-pushed the remove-info-read branch from 85d923c to 1751ddb Compare November 14, 2018 09:11

markuspf merged commit 48047f3 into gap-system:master Nov 14, 2018

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

olexandr-konovalov added this to the GAP 4.11.0 milestone Feb 15, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove DEBUG_LOADING, InfoRead1, and InfoRead2 #2237

Remove DEBUG_LOADING, InfoRead1, and InfoRead2 #2237

markuspf commented Mar 4, 2018 •

edited by fingolfin

Loading

codecov bot commented Mar 4, 2018 •

edited

Loading

fingolfin left a comment

fingolfin Mar 4, 2018

markuspf Mar 4, 2018

fingolfin Mar 10, 2018

fingolfin commented Mar 4, 2018

markuspf commented Mar 4, 2018

ThomasBreuer commented Mar 5, 2018

fingolfin Mar 10, 2018

fingolfin commented Mar 20, 2018

markuspf commented Jun 6, 2018

fingolfin commented Jun 7, 2018

fingolfin commented Jun 10, 2018

olexandr-konovalov commented Sep 29, 2018

fingolfin left a comment

markuspf commented Nov 10, 2018

olexandr-konovalov Nov 10, 2018

olexandr-konovalov Nov 14, 2018 •

edited

Loading

Remove DEBUG_LOADING, InfoRead1, and InfoRead2 #2237

Remove DEBUG_LOADING, InfoRead1, and InfoRead2 #2237

Conversation

markuspf commented Mar 4, 2018 • edited by fingolfin Loading

codecov bot commented Mar 4, 2018 • edited Loading

Codecov Report

fingolfin left a comment

Choose a reason for hiding this comment

fingolfin Mar 4, 2018

Choose a reason for hiding this comment

markuspf Mar 4, 2018

Choose a reason for hiding this comment

fingolfin Mar 10, 2018

Choose a reason for hiding this comment

fingolfin commented Mar 4, 2018

markuspf commented Mar 4, 2018

ThomasBreuer commented Mar 5, 2018

fingolfin Mar 10, 2018

Choose a reason for hiding this comment

fingolfin commented Mar 20, 2018

markuspf commented Jun 6, 2018

fingolfin commented Jun 7, 2018

fingolfin commented Jun 10, 2018

olexandr-konovalov commented Sep 29, 2018

fingolfin left a comment

Choose a reason for hiding this comment

markuspf commented Nov 10, 2018

olexandr-konovalov Nov 10, 2018

Choose a reason for hiding this comment

olexandr-konovalov Nov 14, 2018 • edited Loading

Choose a reason for hiding this comment

markuspf commented Mar 4, 2018 •

edited by fingolfin

Loading

codecov bot commented Mar 4, 2018 •

edited

Loading

olexandr-konovalov Nov 14, 2018 •

edited

Loading