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.

Sign up for GitHub

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

Jump to bottom

Add options to debug package manager #931

Merged
merged 1 commit into from
Sep 18, 2023
Merged

Add options to debug package manager #931

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 27 additions & 13 deletions src/packages.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,14 @@ function init_packagemanager()
# put the new method in the first position
meths = Globals.Download_Methods
Wrappers.Add(meths, GapObj(r, recursive=true), 1)

# monkey patch PackageManager so that we can disable removal of
# package directories for debugging purposes
orig_PKGMAN_RemoveDir = Globals.PKGMAN_RemoveDir
replace_global!(:PKGMAN_RemoveDir, function(dir)
Globals.ValueOption(GapObj("debug")) == true && return
orig_PKGMAN_RemoveDir(dir)
Copy link
Member Author

Choose a reason for hiding this comment

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

Of course we can remove this hack once we are using a PackageManager release with gap-packages/PackageManager#119 or something equivalent in it

end)
end
end

Expand Down Expand Up @@ -186,6 +194,7 @@ end
"""
install(spec::String, version::String = "";
interactive::Bool = true, quiet::Bool = false,
debug::Bool = false,
Copy link
Member Author

Choose a reason for hiding this comment

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

Of course this should also be documented; I'll do that once we settled on what final form this should have.

pkgdir::AbstractString = GAP.Packages.DEFAULT_PKGDIR[])

Download and install the GAP package given by `spec` into the `pkgdir`
Expand Down Expand Up @@ -213,28 +222,30 @@ For details, please refer to its documentation.
"""
function install(spec::String, version::String = "";
interactive::Bool = true, quiet::Bool = false,
debug::Bool = false,
pkgdir::AbstractString = DEFAULT_PKGDIR[])
# point PackageManager to the given pkg dir
Globals.PKGMAN_CustomPackageDir = GapObj(pkgdir)
mkpath(pkgdir)

if quiet
if quiet || debug
oldlevel = Wrappers.InfoLevel(Globals.InfoPackageManager)
Wrappers.SetInfoLevel(Globals.InfoPackageManager, 0)
Wrappers.SetInfoLevel(Globals.InfoPackageManager, quiet ? 0 : 3)
Copy link
Member Author

Choose a reason for hiding this comment

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

I wonder if it might not be better to separate debug and this "verbose" mode: leaving around partial packages could be a stumbling block for non-experts. Yet we may end up asking these very people to use debug if they report issues installing packages. Of course we can ask them to GAP.Packages.remove("blah") afterwards, but still...

So one idea would be to also add verbose::Bool = false as another kwarg. Of course then one can wonder what happens if both quiet and verbose are set (an error?). Seeing how quiet is off by default, perhaps a better alternative would be to add a "verbosity level". So:

  • verbose::Int = 0 is a new kwarg and directly corresponds to the InfoLevel
    • alternative names: verbosity, verbosity_level, infolevel, info_level, ... ?
    • default value is 0 to match the current default of quiet=true
  • we either drop quiet; or keep it as "deprecated" ("please instead use ...")
    • if set to true that sets verbose to 0, if set to false it sets it to whatever is the default value for InfoPackageManager (or whatever value we prefer)
    • implementation: `function foo(quiet::Bool = true, verbose::Int = (quiet ? 0 : 2))
  • thus if both are given, verbose wins (we could of course also use nothing to detect the case and report an error, but I feel this is overthinking it... on the long run I'd just remove quiet)
  • we remove the option of "not changing the infolevel (it is redundant if the user has full control over the info level anyway)

Copy link
Member

Choose a reason for hiding this comment

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

My viewpoint was that PackageManager's default info level is reasonable (otherwise this level should be changed there) but that there are situations where we want to suppress all info messages, for example if a package installation is triggered by the request to load a package. This is why quiet arose instead of access to the various info levels (which are documented in PackageManager).

There is of course no problem with the viewpoint that one should be able to set the info level in each single function call. I do not see why this is helpful here, but it is surely a "conceptual" feature which is currently not supported by PackageManager

(Note that by default, the functions from GAP.Packages except load do show the info messages at the current level --the default for the keyword argument quiet is false for them. Just for load, quiet = true is the default, and the value is handed over to install when load was called with the install = true option.)

Debugging is something different, the current proposal for debug changes the behaviour of the functions in the sense that RemovePackage calls are deliberately omitted.
I think that setting debug = true in the call of a function should imply setting the info level to the highest meaningful value, perhaps except if the info level is explicitly set in the same function call.

end
if version == ""
res = Wrappers.InstallPackage(GapObj(spec), interactive)
res = Globals.InstallPackage(GapObj(spec), interactive; debug)
else
res = Wrappers.InstallPackage(GapObj(spec), GapObj(version), interactive)
res = Globals.InstallPackage(GapObj(spec), GapObj(version), interactive; debug)
end
if quiet
if quiet || debug
Wrappers.SetInfoLevel(Globals.InfoPackageManager, oldlevel)
end
return res
end

"""
update(spec::String; interactive::Bool = true, quiet::Bool = false,
debug::Bool = false,
pkgdir::AbstractString = GAP.Packages.DEFAULT_PKGDIR[])

Update the GAP package given by `spec` that is installed in the
Expand All @@ -253,19 +264,20 @@ prevent `PackageManager` from prompting the user for input interactively.
For details, please refer to its documentation.
"""
function update(spec::String; interactive::Bool = true, quiet::Bool = false,
debug::Bool = false,
pkgdir::AbstractString = DEFAULT_PKGDIR[])
# point PackageManager to the given pkg dir
Globals.PKGMAN_CustomPackageDir = GapObj(pkgdir)
mkpath(pkgdir)

if quiet
if quiet || debug
oldlevel = Wrappers.InfoLevel(Globals.InfoPackageManager)
Wrappers.SetInfoLevel(Globals.InfoPackageManager, 0)
res = Wrappers.UpdatePackage(GapObj(spec), interactive)
Wrappers.SetInfoLevel(Globals.InfoPackageManager, quiet ? 0 : 3)
res = Globals.UpdatePackage(GapObj(spec), interactive; debug)
Wrappers.SetInfoLevel(Globals.InfoPackageManager, oldlevel)
return res
else
return Wrappers.UpdatePackage(GapObj(spec), interactive)
return Globals.UpdatePackage(GapObj(spec), interactive)
end
end
# note that the updated version cannot be used in the current GAP session,
Expand All @@ -274,6 +286,7 @@ end

"""
remove(spec::String; interactive::Bool = true, quiet::Bool = false,
debug::Bool = false,
pkgdir::AbstractString = GAP.Packages.DEFAULT_PKGDIR[])

Remove the GAP package with name `spec` that is installed in the
Expand All @@ -288,19 +301,20 @@ prevent `PackageManager` from prompting the user for input interactively.
For details, please refer to its documentation.
"""
function remove(spec::String; interactive::Bool = true, quiet::Bool = false,
debug::Bool = false,
pkgdir::AbstractString = DEFAULT_PKGDIR[])
# point PackageManager to the given pkg dir
Globals.PKGMAN_CustomPackageDir = GapObj(pkgdir)
mkpath(pkgdir)

if quiet
if quiet || debug
oldlevel = Wrappers.InfoLevel(Globals.InfoPackageManager)
Wrappers.SetInfoLevel(Globals.InfoPackageManager, 0)
res = Wrappers.RemovePackage(GapObj(spec), interactive)
Wrappers.SetInfoLevel(Globals.InfoPackageManager, quiet ? 0 : 3)
res = Globals.RemovePackage(GapObj(spec), interactive; debug)
Wrappers.SetInfoLevel(Globals.InfoPackageManager, oldlevel)
return res
else
return Wrappers.RemovePackage(GapObj(spec), interactive)
return Globals.RemovePackage(GapObj(spec), interactive)
end
end

Expand Down
4 changes: 0 additions & 4 deletions src/wrappers.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,6 @@ import GAP: @wrap
@wrap IN(x::Any, y::Any)::Bool
@wrap InfoLevel(x::GapObj)::Int
@wrap INT_CHAR(x::Any)::Int
@wrap InstallPackage(x::GapObj, y::Bool)::Bool
@wrap InstallPackage(x::GapObj, y::GapObj, z::Bool)::Bool
@wrap InverseSameMutability(x::Any)::Any
@wrap IS_JULIA_FUNC(x::Any)::Bool
@wrap ISB_LIST(x::Any, i::Int)::Bool
Expand Down Expand Up @@ -68,7 +66,6 @@ import GAP: @wrap
@wrap QUO(x::Any, y::Any)::Any
@wrap Read(x::GapObj)::Nothing
@wrap RecNames(x::Any)::Any
@wrap RemovePackage(x::GapObj, y::Bool)::Bool
@wrap RNamObj(x::Any)::Int
@wrap SetInfoLevel(x::GapObj, y::Int)::Nothing
@wrap SetPackagePath(x::GapObj, y::GapObj)::Nothing
Expand All @@ -79,7 +76,6 @@ import GAP: @wrap
@wrap StructuralCopy(x::Any)::Any
@wrap SUM(x::Any, y::Any)::Any
@wrap UNB_REC(x::GapObj, y::Int)::Nothing
@wrap UpdatePackage(x::GapObj, y::Bool)::Bool
@wrap ZeroSameMutability(x::Any)::Any

end