oscar-system / Oscar.jl

A comprehensive open source computer algebra system for computations in algebra, geometry, and number theory.
https://www.oscar-system.org
Other
307 stars 112 forks source link

undo most of #3874 #3890

Closed ThomasBreuer closed 4 hours ago

ThomasBreuer commented 4 days ago

Return to the situation where the released version of GAP's recog package gets installed if necessary, not a current version from github.

We keep the change that the OscarInterface package gets loaded in the end, and that it requires the Polycyclic package.

I think that once there will be a new released recog version, the install call for recog in Oscar will run into an error unless we are then able to start GAP without loaded packages. The point is that if a version is already loaded that lies in a path different from the path of the newly installed version, the GAP code in recog that gets called for processing the documentation will throw an error. This behaviour of recog is not nice, the people who invented it did probably not think of the situation that the package gets installed automatically by PackageManager.

fingolfin commented 3 days ago

Thank you!

This behaviour of recog is not nice, the people who invented it did probably not think of the situation that the package gets installed automatically by PackageManager

They absolutely did not, because when I added regen_doc.g in 2017, PackageManager did not even exist :-). And to this day nobody apparently tried to install recog via PackageManager either (or at least they did not bother to write an issue for recog), otherwise "we" would have looked into fixing it long ago. (Though looking at the changes you made, they seem to me more like a workaround for a bug in PackageManager than a fix? Note that makedoc.g in virtually all GAP packages really expects to be run from the directory it lives in; that it apparently often works otherwise as well is just luck (whether good or bad luck is a matter of opinion, after all it hid the bug in PackageManager for a long time)

codecov[bot] commented 4 hours ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 83.87%. Comparing base (5acca40) to head (2fce1d5). Report is 3 commits behind head on master.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #3890 +/- ## ======================================= Coverage 83.87% 83.87% ======================================= Files 589 588 -1 Lines 80864 80831 -33 ======================================= - Hits 67823 67797 -26 + Misses 13041 13034 -7 ``` | [Files](https://app.codecov.io/gh/oscar-system/Oscar.jl/pull/3890?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=oscar-system) | Coverage Δ | | |---|---|---| | [src/Oscar.jl](https://app.codecov.io/gh/oscar-system/Oscar.jl/pull/3890?src=pr&el=tree&filepath=src%2FOscar.jl&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=oscar-system#diff-c3JjL09zY2FyLmps) | `67.53% <ø> (-1.22%)` | :arrow_down: |