Metaxal / quickscript

Easy scripting for DrRacket
Other
17 stars 6 forks source link

`register.rkt` breaks build in some environments #73

Open LiberalArtist opened 1 year ago

LiberalArtist commented 1 year ago

The addition of register.rkt in https://github.com/Metaxal/quickscript/pull/72 breaks building Quickscript (as part of the Racket 8.11 release) in Guix, because, in our build environment, the current user's home directory does not exist and cannot be created.

I can work around this by setting PLTUSERHOME or patching out register.rkt (which is what I plan to do, to avoid masking other errors), but, as I've looked into the underlying problem, I don't think register.rkt is really working as intended in any case. The problem is even broader because the documentation recommends this approach:

Here is the code: https://github.com/Metaxal/quickscript/blob/5243a8740a6f22732f694427818bb3b3b9e363e5/register.rkt#L6-L10

AIUI, it tries to write to library-file as a side-effect when the module register.rkt is visited in the sense of "Module Expansion, Phases, and Visits". Since register.rkt is otherwise unused, the module is visited only when it is compiled by raco setup. But this means that the side-effect only happens for the user who is compiling the module. It seems the side-effect won't happen at all for someone who installed Quickscript as part of a built Racket release.

For scripts distributed as Racket packages, it seems like a better mechanism would be to specify an info.rkt key for packages to define and to use find-relevant-directories.

Some other questionable things I noticed in the process:

Metaxal commented 1 year ago

Thanks for the report and the insights!

I'll use byte-strings for paths in the library. (Though I can't see a case where using the local representation would fail, because that would mean sharing a file written for one OS to another OS where DrR is running, which seems rather implausible, and asking for trouble, if at all possible. That's an easy fix anyway.)

This register business is annoying indeed, I need to replace it. I didn't know about find-relevant-directories — this may be a good option.

Regarding cleaning up, the racket uninstall doesn't clean up the prefs and config directories either, because otherwise preferences would be lost when installing a new version (which asks for uninstalling the previous one). I'm not sure what you are suggesting here though. From my point of view as a user, sharing the QS library between Racket versions and installations is the intended behaviour. If you have other use cases, let me know.

Metaxal commented 1 year ago

@spdegabrielle FYI

LiberalArtist commented 1 year ago

I'll use byte-strings for paths in the library. (Though I can't see a case where using the local representation would fail, because that would mean sharing a file written for one OS to another OS where DrR is running, which seems rather implausible, and asking for trouble, if at all possible. That's an easy fix anyway.)

Cases where strings don't work are admittedly strange, but there are paths which are not representable as strings:

Regarding cleaning up, the racket uninstall doesn't clean up the prefs and config directories either, because otherwise preferences would be lost when installing a new version (which asks for uninstalling the previous one). I'm not sure what you are suggesting here though. From my point of view as a user, sharing the QS library between Racket versions and installations is the intended behaviour. If you have other use cases, let me know.

For scripts that are not installed using the Racket package system, that sharing seems right.

The scenario I was envisioning is that a user of Racket 8.9 does raco pkg install quickscript-extra and then uses raco pkg migrate to install it in Racket 8.19 and 8.11. If the "registration" actually worked, I think this user would end up with multiple entries in the library, something like:

That's part of why I think that, for scripts distributed in Racket packages, "registration" should use a mechanism like find-relevant-directories that integrates with the package system.

This register business is annoying indeed, I need to replace it. I didn't know about find-relevant-directories — this may be a good option.

I'd be willing to draft a PR, potentially. The aspects I'm least sure of are what constitutes Quickscript's "public API" and what the compatibility considerations are.

Metaxal commented 1 year ago

Cases where strings don't work are admittedly strange, but there are paths which are not representable as strings:

  • On Windows, a path is a sequence of Unicode code units, potentially including unpaired surrogates.
  • On Unix, a path is a sequence of bytes which only hopefully form a valid string under the current locale's encoding.

:+1:

For scripts that are not installed using the Racket package system, that sharing seems right.

:+1:

The scenario I was envisioning is that a user of Racket 8.9 does raco pkg install quickscript-extra and then uses raco pkg migrate to install it in Racket 8.19 and 8.11. If the "registration" actually worked, I think this user would end up with multiple entries in the library, something like: [...]

This is correct indeed, see here

That's part of why I think that, for scripts distributed in Racket packages, "registration" should use a mechanism like find-relevant-directories that integrates with the package system.

This register business is annoying indeed, I need to replace it. I didn't know about find-relevant-directories — this may be a good option.

I'd be willing to draft a PR, potentially.

Awesome!

The aspects I'm least sure of are what constitutes Quickscript's "public API" and what the compatibility considerations are.

Since this mechanism is broken anyway, I think it's fair to consider changing it a bug fix.

There are only 3 cases I know of that use this registration mechanism: quickscript itself, quickscript-extra, quickscript-competition-2020, which are all managed by Stephen and myself.

LiberalArtist commented 1 year ago

That's part of why I think that, for scripts distributed in Racket packages, "registration" should use a mechanism like find-relevant-directories that integrates with the package system.

This register business is annoying indeed, I need to replace it. I didn't know about find-relevant-directories — this may be a good option.

I'd be willing to draft a PR, potentially.

Awesome!

The aspects I'm least sure of are what constitutes Quickscript's "public API" and what the compatibility considerations are.

Since this mechanism is broken anyway, I think it's fair to consider changing it a bug fix.

There are only 3 cases I know of that use this registration mechanism: quickscript itself, quickscript-extra, quickscript-competition-2020, which are all managed by Stephen and myself.

In that case, maybe we can greatly simplify the library mechanism. It seems like we need:

  1. Locations configured through the package system
  2. user-script-dir

Are there any other kinds of locations needed? It seems like we could defer almost all configuration to the package system, especially if we make it convenient.

Metaxal commented 1 year ago

In that case, maybe we can greatly simplify the library mechanism. It seems like we need:

  1. Locations configured through the package system
  2. user-script-dir

Are there any other kinds of locations needed? It seems like we could defer almost all configuration to the package system, especially if we make it convenient.

Things may be a little more complicated than this though. The library allows to add specific user directories (such as the user-scripts by default), remove directories (such as those installed by packages), and disable individual scripts.

There's one other piece that needs to be taken into account: shadow scripts Imagine a user installs a QS package pack (such as quickscript-extra). They want to tweak one of its scripts pack/a.rkt in there to their needs, but modifying the file may be either impossible (permission denied) or would be undone as soon as the package is updated (say with bug fixes). To avoid touching pack/a.rkt, the user can create a shadow script from the library. This creates a new script a.rkt in the user's user-scripts directory, which merely calls pack/a.rkt, with the difference that user-scripts/a.rkt can be modified by the user without the issues mentioned above. The original script pack/a.rkt is also disabled in the library.

Currently, script directories in the library are simply a set of 'known' paths. If we are to use the info.rkt mechanism, we would need to augment the library with 'known paths that are disabled'. Otherwise there would be no other option for the user but to uninstall the package altogether — which may either be not possible or not convenient, e.g., if the user needs some features for coding with require.

Or maybe you're thinking of something else?

rfindler commented 12 months ago

I just ran into an issue with quickscripts and the issues of finding scripts in another context and it may also be relevant to the discussion here. Specifically, in my development (git-based) build of drracket, I recently got this error:

Run: Error in script file "/Applications/Racket v8.6 qi/share/pkgs/Qi-Quickscripts/scripts/insert-qi.rkt":
 loading code: version mismatch
  expected: "8.11.0.3"
  found: "8.11.0.900"
  in: /Applications/Racket v8.6 qi/share/pkgs/Qi-Quickscripts/scripts/compiled/insert-qi_rkt.zo
  possible solution: running `racket -y`, `raco make`, or `raco setup`
  context...:
   /Users/robby/git/exp/plt/extra-pkgs/quickscript/tool.rkt:228:12
   /Users/robby/git/exp/plt/extra-pkgs/quickscript/tool.rkt:188:8: run-script method in script-menu-mixin
   /Users/robby/git/exp/plt/extra-pkgs/gui/gui-lib/mred/private/mrmenu.rkt:250:14: command method in basic-selectable-menu-item%
   /Users/robby/git/exp/plt/racket/collects/racket/private/more-scheme.rkt:148:2: call-with-break-parameterization
   /Users/robby/git/exp/plt/racket/collects/ffi/unsafe/atomic.rkt:73:13
   /Users/robby/git/exp/plt/extra-pkgs/gui/gui-lib/mred/private/wx/common/queue.rkt:436:6
   /Users/robby/git/exp/plt/extra-pkgs/gui/gui-lib/mred/private/wx/common/queue.rkt:487:32
   /Users/robby/git/exp/plt/extra-pkgs/gui/gui-lib/mred/private/wx/common/queue.rkt:639:3

What seems to be happening here is that an old version installed in my /Applications directory (that is, a system wide place where applications are installed in macos) I have installed the qi package, but in an installation-specific way. That installation of qi seems to have its quickscripts being picked up by the git build version which seems undesirable.

It could make sense if qi had been installed in a global place (but then the path in the error message, pointing at qi, would have been different), but since it wasn't, it seems like quickscript shouldn't be looking for quickscripts there.

spdegabrielle commented 12 months ago

Quickscripts are intended to be shared among all Racket versions and installations.

This has historically meant that the first thing that happens when you upgrade is a wrong version error because scripts were compiled for a different version of racket

Register is intended to install ‘open terminal here’ and ‘eyes’ commands as part of the base install. Sadly this does not work (I thought I tested it successfully but I managed to fool myself)

i will do an immediate PR to remove register.rkt

Going forward I am wondering if installation via the package system should be retired given there is an alternate installation mechanism?

rfindler commented 12 months ago

Quickscripts are intended to be shared among all Racket versions and installations.

Going forward I am wondering if installation via the package system should be retired given there is an alternate installation mechanism?

It seems to me that quickscripts via pkgs is very useful!

Perhaps there should be a distinction between quickscripts that come in via the user using the GUI and quickscripts that come in via a pkg installation. If they are coming with the package, then they follow the same rules for finding code in the package (which can be accomplished by using the setup/get-info library) and if they are being installed by the user directly via the quickscript GUI in DrRacket, they can be more global?

spdegabrielle commented 12 months ago

I'm assuming you mean using (find-relevant-directories syms [mode]) to identify script locations e.g.

#lan info
[...]
(define scripts '(("scripts/" ()))) ; script directories relative to the collection path (like scribblings)

If I understand correctly this doesn't resolve the Guix problem because Guix expects packages to be immutable for good reasons.

I agree installation via package is useful and I was wrong the alternate method of installation is url2script/fetch script is a script - so is not available on installation - you have to install the quickscript-extra package to use it.

Package installation of scripts is not without it's problems; I mentioned that starting DrRacket after an upgrade is met with compilation errors; another problem is #lang info (define deps ... is ignored. So you can install quick script-competition-2020 and still get a compile error complaining about a missing search-list-box package.

It is a constant disappointment to me that new users of DrRacket almost never discover the scripts functionality that @Metaxal created for DrRacket.

I'm always looking for social and technical solutions to this problem. The Qi-Quickscripts package is one approach. Quickscripts competition(2020) was another.

I really thought I hit on a winner with Add register.rkt eyes and open terminal - real scripts that are useful and demonstrate some of the capabilities 'out of the box' - but as I mentioned before I failed to test it effectively and fooled myself into thinking register.rkt would work on the quickscript package itself. :-(

I'm home tonight so - instead of watching television - I'll compose an 'Opinionated guide to scripting DrRacket', and try work out how I can get the core library to copy scripts direct into the user ~/Library/Preferences/quickscript/user-scripts

LiberalArtist commented 12 months ago

Package installation of scripts is not without it's problems; I mentioned that starting DrRacket after an upgrade is met with compilation errors; another problem is #lang info (define deps ... is ignored. So you can install quick script-competition-2020 and still get a compile error complaining about a missing search-list-box package.

I think we should solve these problems "not by piling feature on top of feature, but by removing the weaknesses and restrictions that make additional features appear necessary."

It seems like we have identified several problems with register.rkt as an approach for a Racket package to declare that it provides Quickscript scripts:

  1. In some environments, it breaks the build.
  2. In other environments, it doesn't raise any errors, but it also doesn't do what it attempts to.
  3. It interacts poorly with raco pkg migrate, creating duplicate configuration entries (https://github.com/Metaxal/quickscript-extra/issues/27) and perhaps other issues.

The solution to these problems seems to be making better use of existing features—in particular, the setup/get-info library—to provide a better mechanism for packages to declare that they provide Quickscripts. I think I have a sense from @Metaxal's https://github.com/Metaxal/quickscript/issues/73#issuecomment-1817434374 of what an implementation would look like. I need to finish up some other things before I start writing code, but I plan to take a look at it soon.

To the extent that I understand it, I think a better approach here would also solve this aspect of https://github.com/Metaxal/quickscript/issues/73#issuecomment-1832405523:

another problem is #lang info (define deps ... is ignored. So you can install quick script-competition-2020 and still get a compile error complaining about a missing search-list-box package.

The scenario I'm imaging when that would happen is that you install some package providing Quickscripts, which would install its dependencies as usual, and then you update Racket, but you don't run raco pkg migrate. Currently, Quickscript would still try to use the scripts from the package for old Racket installation, but, because Quickscript is managing this state outside of the package system, nothing would trigger the dependencies to be installed for the new Racket installation.

While I think a replacement for register.rkt would solve the kind of version mismatch @rfindler reported in https://github.com/Metaxal/quickscript/issues/73#issuecomment-1832063561 (i.e. trying to use a script from a package installed into a different Racket installation), I think that's distinct from, or at most a special case of, the (more common?) version issue @spdegabrielle noted in https://github.com/Metaxal/quickscript/issues/73#issuecomment-1832109040:

This has historically meant that the first thing that happens when you upgrade is a wrong version error because scripts were compiled for a different version of racket

I think this issue affects scripts in user-script-dir that are compiled by Quickscript, rather than raco setup, because they are not part of any package. This problem is also worth solving, but I think the solution would be different. Maybe it could adjust use-compiled-file-paths to allow multiple versions to coexist? Maybe it should just automatically recompile when there's a version mismatch?

Metaxal commented 12 months ago

Just to be clear, I think that:

Additionally, note that

Metaxal commented 12 months ago

and also I'm definitely happy to discuss a new proposal even if it doesn't check all the boxes for now!

Metaxal commented 11 months ago

While the main issue raised in this report is fixed by fe3040dede7c952d6b310e7302cc73009e355ba8, I'll keep this thread open as it goes much deeper than just quickscript's own register.rkt issue.

Metaxal commented 10 months ago

Idea (based on the above): Every collection such as quickscript-extra defines the symbol quickscript-collection in its info.rkt. When QS starts, or on a particular user action (e.g., Scripts|Manage|Library), QS searches for the collection with the said symbol using find-relevant-directories and a further get-info/full (to make sure the symbol is indeed there).

Then it open a dialog to ask the user if they want to add the found collection to the list of paths in the library, with a dialog box and checkboxes.

This way we retain most of the previous code and the flexibility of the library while avoiding the faulty register! mechanism. How does this sound @LiberalArtist ?

LiberalArtist commented 10 months ago

I've started prototyping something somewhat similar in spirit.

Then it open a dialog to ask the user if they want to add the found collection to the list of paths in the library, with a dialog box and checkboxes.

I think it would be better to avoid requiring a separate step to actually make scripts available that were installed from a package. That would complicate matters for a sysadmin or distributor trying to make certain scripts available by default. If a user doesn't want specific scripts, they already have the option to exclude them.

LiberalArtist commented 10 months ago
  • An info.rkt file defines quickscript-paths as either 'all (meaning use the directory containing the info.rkt file) or a (list/c path-string? regexp?) for paths relative to the directory. (This seems like more flexibility than is really needed here, but it would be consistent with test-include-paths, compile-omit-paths, and other similar things. Thoughts?)

I could also imagine (define quickscript-directory #t) and document that values other than #t are reserved for future use.

LiberalArtist commented 10 months ago

Since this is going to involve a change to the saved library format, how would you feel about adopting the framework/preferences system? It can take care of locking, future extensibility, etc. If supporting the undocumented PLTQUICKSCRIPTDIR is important, I think that would work, but, if it seems ok to just use the default preferences file (which can be customized via PLTUSERHOME), that would be even easier.

rfindler commented 10 months ago

I think it is a good idea to use the framework preferences system (it is heavily used by DrRacket already).

Robby

LiberalArtist commented 10 months ago

I've opened https://github.com/Metaxal/quickscript/pull/81 with some work in progress.

Metaxal commented 10 months ago

I could also imagine (define quickscript-directory #t) and document that values other than #t are reserved for future use.

Then maybe just (define quickscript-directory "scripts") instead?

When QS starts, it finds all of the specified collection-based scripts via find-relevant-directories and get-info/full. These are conceptually part of the "library", but they are not stored as part of the data that QS persists on-disk, so they are tied to the installation for which the package is installed, are not duplicated when migrating versions, etc.

Good.

One issue though: quickscript-test comes with a bunch a scripts that should loaded only when running the test suite. So these scripts must be excluded by default. How could this happen? (Currently, these scripts are not included by default, and registered only for the duration of the tests.)

The new mechanism also requires the creation of a collection to be able to add a directory with scripts. It's heavier than the un/register approach and can't be done (easily) directly within the library GUI I suspect.

I suppose we could have both: (i) explicit exclusions of find-relevant-directories and (ii) explicit inclusions of user-added paths.

(i) would be for collections and (ii) would be for non-collections.

Thoughts?

I think it would be better to avoid requiring a separate step to actually make scripts available that were installed from a package. That would complicate matters for a sysadmin or distributor trying to make certain scripts available by default. If a user doesn't want specific scripts, they already have the option to exclude them.

:+1:

Reminders:

LiberalArtist commented 10 months ago

I'll try to reply to this in https://github.com/Metaxal/quickscript/pull/81 to keep discussion in one place.