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.

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

register.rkt breaks build in some environments #73

Open
LiberalArtist opened this issue Nov 15, 2023 · 22 comments
Open

register.rkt breaks build in some environments #73

LiberalArtist opened this issue Nov 15, 2023 · 22 comments

Comments

@LiberalArtist
Copy link
Contributor

The addition of register.rkt in #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:

;; This is going to be called during setup and will automatically
;; register quickscript-extra in quickscript's library.
(begin-for-syntax
(define-runtime-path script-dir "scripts")
(add-third-party-script-directory! script-dir))

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:

  • Assuming PLTQUICKSCRIPTDIR is not set, library-file will be (build-path (find-system-path 'prefs-dir) "quickscript/library.rktd"), so it is shared among all Racket versions and installations. While register.rkt attempts to register a new directory on setup, there seems to be no attempt to clean up on uninstallation—and any such attempt would be unreliable, anyway, since "uninstallation" might mean rm -r.
  • The representation of a library represents paths as strings, which has the usual issues: if serialization is the issue, it seems like byte strings or racket/fasl would be more robust alternatives.
@Metaxal
Copy link
Owner

Metaxal commented Nov 17, 2023

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
Copy link
Owner

Metaxal commented Nov 17, 2023

@spdegabrielle FYI

@LiberalArtist
Copy link
Contributor Author

LiberalArtist commented Nov 17, 2023

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:

  • 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.

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:

  • ~/.local/share/racket/8.9/pkgs/quickscript-extra/scripts
  • ~/.local/share/racket/8.10/pkgs/quickscript-extra/scripts
  • ~/.local/share/racket/8.11/pkgs/quickscript-extra/scripts

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
Copy link
Owner

Metaxal commented Nov 17, 2023

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.

👍

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: [...]

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
Copy link
Contributor Author

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
Copy link
Owner

Metaxal commented Nov 18, 2023

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
Copy link
Collaborator

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
Copy link
Contributor

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
Copy link
Collaborator

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
Copy link
Contributor

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
Copy link
Contributor Author

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 (Auto-update only adds to library path quickscript-extra#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 #73 (comment) 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 #73 (comment):

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 #73 (comment) (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 #73 (comment):

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
Copy link
Owner

Metaxal commented Dec 1, 2023

Just to be clear, I think that:

  • being able to install quickscripts from packages is very useful and should be kept. The intent here is that such scripts can be of 'production' quality, possibly with documentation and tests.
  • being able to install individual scripts into the user's quickscript directory is also very useful and should be kept. The intent here is to make it easy to write quick-and-dirty scripts and share them easily. The url2script mechanism helps with this. (If people can't write quick-and-dirty scripts, they will just not write scripts at all. This applies in particular to one-off scripts that you can write for a particular and temporary purpose.)
  • the current register mechanism is bad for packages and should be replaced with something more reliable when it comes to package-based quickscripts. In particular the dependency issue is already solved by packages and raco setup so we should definitely use that. Though perhaps this mechanism should be kept for user-script directories.
  • The library itself is very useful: It allows to disable individual scripts, whether in user-scripts or in packages, add specific directories and create shadow scripts. Something like this should be kept — unless we find an even better solution.

Additionally, note that

  • quickscripts in the user-scripts directory are automatically recompiled on DrR's startup, and each time they are launched, in case there's a version mismatch. This doesn't apply to packages.
  • I'm also fine with the fact that if a script in user-scripts relies on a package that isn't properly installed it just raises an error, just like running a file with the same problem in DrRacket raises the same error — but I don't think it's fine for packages. Error reporting, help and QS' behaviour can still be improved in this case anyway.
  • The user can manually add/register their own script directory via the library. My view on this is that if a user adds such a directory A, than all scripts in A (apart from the excluded ones) should be available for this user for all installations of Racket. This may not apply to package-based scripts.
  • Shadow scripts build a bridge between package-based scripts — which are essentially immutable — and user-modified scripts. This mechanism should be kept and maintained — unless we can find something better.

@Metaxal
Copy link
Owner

Metaxal commented Dec 1, 2023

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

@Metaxal
Copy link
Owner

Metaxal commented Dec 10, 2023

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

@Metaxal
Copy link
Owner

Metaxal commented Jan 5, 2024

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
Copy link
Contributor Author

I've started prototyping something somewhat similar in spirit.

  • 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?)
  • 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.
  • UI like Scripts|Manage|Library allows the user to exclude scripts installed via this mechanism, and the exclusions are persisted by QS (using path->collects-relative and/or path->pkg+subpath), so the user preference persists across upgrades. Maybe we should also consider UI to show and allow for removing exclusions that don't correspond to an installed collection-based script.

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
Copy link
Contributor Author

  • 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
Copy link
Contributor Author

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
Copy link
Collaborator

rfindler commented Jan 6, 2024 via email

LiberalArtist added a commit to LiberalArtist/quickscript that referenced this issue Jan 6, 2024
LiberalArtist added a commit to LiberalArtist/quickscript that referenced this issue Jan 6, 2024
LiberalArtist added a commit to LiberalArtist/quickscript that referenced this issue Jan 6, 2024
LiberalArtist added a commit to LiberalArtist/quickscript that referenced this issue Jan 6, 2024
LiberalArtist added a commit to LiberalArtist/quickscript that referenced this issue Jan 7, 2024
LiberalArtist added a commit to LiberalArtist/quickscript that referenced this issue Jan 7, 2024
LiberalArtist added a commit to LiberalArtist/quickscript that referenced this issue Jan 7, 2024
@LiberalArtist
Copy link
Contributor Author

I've opened #81 with some work in progress.

@Metaxal
Copy link
Owner

Metaxal commented Jan 8, 2024

  • I'm okay with using the preference system. I'm just worried that the prefs file is getting big and any change of any pref requires rewriting the whole file, which becomes slower and slower as we add more prefs. Or can we use a separate quickscripts-prefs.rktd file maybe?

  • I'm okay with keeping things consistent with test-include-paths and friends, except that all doesn't seem adequate for quickscripts (info.rkt is not a script for example), and it seems to me that the default should be a scripts subdirectory instead — or automatically skipping files like info.rkt but that seems fragile.

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.

👍

Reminders:

  • Do keep in mind how to create shadow scripts from existing collection-based scripts. (I don't think I have a test for this though, since it's a little tricky as it depends on adding ad-hoc collections and stuff. Not a good excuse anyhow.)
  • The library should still show all the scripts that are loaded or excluded.

@LiberalArtist
Copy link
Contributor Author

I'll try to reply to this in #81 to keep discussion in one place.

Metaxal pushed a commit that referenced this issue Jan 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants