racket / drracket

DrRacket, IDE for Racket
http://www.racket-lang.org/
Other
445 stars 93 forks source link

Move non-GUI parts of drracket-tool-lib to new drracket-tool-text-lib #520

Closed greghendershott closed 2 years ago

greghendershott commented 2 years ago

This is to follow up on issue #517.

Not sure if the ball was in my court. I wanted to (hopefully) help with at least a draft PR to be more specific about the proposal. Although this isn't urgent, for me, I didn't want it to fall between the cracks.


This PR moves the non-GUI parts of drracket-tool-lib to a new drracket-tool-text-lib directory, and in the former's info.rkt adds a dep on the latter.

The non-GUI parts turn out to be effectively "everything except the GUI module browser".

WARNING: I'm not sure I understand the correct roll-out steps here.

This PR assumes the steps are simply:

  1. Create a new drracket-tool-text-lib package on pkgs.racket-lang.org (which should be under @rfindler 's account there, not mine, correct?).
  2. Merge this PR.

But would that leave an inconsistent window?

Instead should it be something like the following?

  1. Do one PR which simply copies the non-GUI code to the new directory for the new package (temporarily leaving copies of the non-GUI code in both).
  2. Create the new package catalog entry.
  3. Do another PR which deletes the non-GUI code from drracket-tool-lib and updates its info.rkt to depend on drracket-tool-text-lib (of course leaving the copy there).
rfindler commented 2 years ago

Thanks, @greghendershott I'm sorry for being slow here. The ball is in my court and I'll try to get this done soon.

rfindler commented 2 years ago

Looks like we also need these changes:

--- a/drracket-tool-lib/info.rkt
+++ b/drracket-tool-lib/info.rkt
@@ -8,10 +8,13 @@
                ["string-constants-lib" #:version "1.12"]
                "scribble-lib"
                "racket-index"
-               "gui-lib"))
+               "gui-lib"
+               "drracket-tool-text-lib"))
 (define build-deps '("at-exp-lib"
                      "rackunit-lib"))

+(define implies '("drracket-tool-text-lib"))
+
 (define pkg-desc "GUI code implementing programmatic interfaces to some IDE tools that DrRacket supports")

 (define pkg-authors '(robby))
diff --git a/drracket-tool-text-lib/info.rkt b/drracket-tool-text-lib/info.rkt
index 0efc326a..893fd20c 100644
--- a/drracket-tool-text-lib/info.rkt
+++ b/drracket-tool-text-lib/info.rkt
@@ -6,7 +6,8 @@
                "scribble-lib"
                ["string-constants-lib" #:version "1.12"]
                "scribble-lib"
-               "racket-index"))
+               "racket-index"
+               "gui-lib"))

I'll go ahead and add the package and push the revised commit, if that's okay with you @greghendershott ?

greghendershott commented 2 years ago

Thanks!

--- a/drracket-tool-lib/info.rkt
+++ b/drracket-tool-lib/info.rkt
@@ -8,10 +8,13 @@
                ["string-constants-lib" #:version "1.12"]
                "scribble-lib"
                "racket-index"
-               "gui-lib"))
+               "gui-lib"
+               "drracket-tool-text-lib"))
 (define build-deps '("at-exp-lib"
                      "rackunit-lib"))

+(define implies '("drracket-tool-text-lib"))
+
 (define pkg-desc "GUI code implementing programmatic interfaces to some IDE tools that DrRacket supports")

 (define pkg-authors '(robby))
  1. I thought I already had the first change but :+1:.

  2. Ugh, I overlooked the second change, adding implies. :+1:

--- a/drracket-tool-text-lib/info.rkt
+++ b/drracket-tool-text-lib/info.rkt
@@ -6,7 +6,8 @@
                "scribble-lib"
                ["string-constants-lib" #:version "1.12"]
                "scribble-lib"
-               "racket-index"))
+               "racket-index"
+               "gui-lib"))
  1. But the last change doesn't seem right? :question: We want drracket-tool-text-lib not to depend on gui-lib (that was the point of splitting it off).
rfindler commented 2 years ago

Ah, right, oops! :)

It looks like I had to move get-module-path.rkt back to drracket-tool-lib (and change the require of find-module-path-completions to an absolute one). Does that sound okay? The file's content definitely appears to be racket/gui territory.

rfindler commented 2 years ago

Here's the revised commit.

greghendershott commented 2 years ago

Yes; although I use find-module-path-completions, I don't use the GUI dialog around it.

So that seems correct/perfect!

rfindler commented 2 years ago

I've merged the commit. Thanks!

greghendershott commented 2 years ago

It looks like drracket-tool-text-lib has a build failure:

The time is now Saturday, October 30th, 2021 6:38:16pm
(/usr/bin/env DISPLAY=:1 PLT_PKG_BUILD_SERVICE=1 PLTUSERHOME=/home/root//user PLT_PKG_BUILD_SERVICE=1 CI=true PLTSTDERR=debug@pkg error PLT_INFO_ALLOW_VARS=;PLT_PKG_BUILD_SERVICE PLTCOMPILEDROOTS=/home/root//zo: /usr/bin/xvfb-run -n 1 /bin/sh -c cd "/home/root/"/racket && bin/racket -MCR "/home/root/"/zo: -l- raco pkg install -u --auto drracket-tool-text-lib)
Resolved "drracket-tool-text-lib" via file:///home/root//catalogs/archive/catalog
pkg: catalog response: #hash((author . "robby@racket-lang.org") (checksum . "33fb77f5fbfc42db0485d1b1786a2cee16c2ea96") (dependencies . (("base" #:version "6.2.900.10") ("scribble-lib") ("string-constants-lib" #:version "1.12") ("scribble-lib") ("racket-index") ("at-exp-lib") ("rackunit-lib"))) (description . "Code implementing non-GUI programmatic interfaces to some IDE tools that DrRacket supports") (modules . ((lib "drracket/check-syntax.rkt") (lib "drracket/private/syncheck/annotate.rkt") (lib "drracket/private/syncheck/syncheck-intf.rkt") (lib "drracket/private/syncheck/syncheck-local-member-names.rkt") (lib "drracket/private/syncheck/xref.rkt") (lib "drracket/private/syncheck/traversals.rkt") (lib "drracket/private/syncheck/colors.rkt") (lib "drracket/find-module-path-completions.rkt") (lib "drracket/private/syncheck/contract-traversal.rkt"))) (name . "drracket-tool-text-lib") (ring . 2) (source . "file:///home/root/catalogs/archive/pkgs/drracket-tool-text-lib.zip") (tags . ()))
racket pkg install: packages conflict
  package: drracket-tool-text-lib
  package: drracket-tool-lib
  module path: drracket/private/syncheck/contract-traversal
The time is now Saturday, October 30th, 2021 6:38:19pm

If I understand the message, it's because it thinks drracket-tool-lib already installed drracket/private/syncheck/contract-traversal?

Is that because drracket-tool-lib did not get rebuilt and was used from a pre-existing cache? As I type this, its package page has a blank (no link) for "Most recent build results", which seems to support this theory (?).

greghendershott commented 2 years ago

Speaking of package pages, drracket-tool-lib has Ring = 0 and Tags = main-distribution.

Whereas drracket-tool-text-lib has Ring = 2 and blank Tags.

Since the former depends on the latter, this seems wrong? Should drracket-tool-text-lib also be Ring = 0 and Tags = main-distribution?

Although I don't know exactly how all this works, it seems this could be a reason for the build problem in the previous comment?

jeapostrophe commented 2 years ago

I believe that on @mflatt has the answer to why the builds are failing. One hypothesis is that the pkg-build is not using master, but is using the previous release. As far as the ring number, etc, while I do manage the ring status of packages, the ones that are part of main-distribution are controlled by another entry point that is part of the release process. This is further evidence for me that we need to wait until after the release for this to happen.

rfindler commented 2 years ago

I'm not completely following, @jeapostrophe -- this change should be post release in the sense that the release branches are all already created and this change is not part of them. Are you saying that any change that affects which pkgs depend on which ones needs to be synchronized with the release somehow?

jeapostrophe commented 2 years ago

I believe that this question is "A. There's a build failure; B. The package info is weird; Why?"

For B, the answer is that main-distribution is controlled via a special path called api/upload which happens as part of releases --- https://github.com/racket/pkg-index/blob/master/official/dynamic.rkt#L78 --- by the libs repo --- https://github.com/racket/libs/blob/master/pack-and-upload.rkt or maybe pkg-push --- https://github.com/racket/pkg-push/blob/master/push-catalog.rkt or maybe release-catalog --- https://github.com/racket/release-catalog

For A, I think the answer is that the "release catalog" is used to initialize pkg-build --- https://github.com/racket/pkg-build --- rather than master, so it won't see things that haven't been in a release yet.

rfindler commented 2 years ago

A: Oh, I think I see. So the fact that there is a failure at the link @greghendershott mentioned is more of a problem that it noticed that the pkg existed (because I had to register it and cannot register it separately for the git-based or snapshot builds than I can for the release) at all than a build failure per se?

But there is a different variant of A, wondering why the snapshot build is also failing?

jeapostrophe commented 2 years ago

I believe that these problems are outside of my expertise.

I know that pkg-build (and I think snapshot) cares about the ring --- https://github.com/racket/pkg-build/blob/master/main.rkt#L1150 --- but I don't think it is safe for me to switch them because pkg-build is supposed to be testing the current release, so we actually don't want to switch yet.