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

Delete nasdf from Nyxt sources #2981

Closed
aadcg opened this issue May 24, 2023 · 16 comments · Fixed by #3262
Closed

Delete nasdf from Nyxt sources #2981

aadcg opened this issue May 24, 2023 · 16 comments · Fixed by #3262
Assignees
Labels

Comments

@aadcg
Copy link
Member

aadcg commented May 24, 2023

@Ambrevar could you provide a step-by-step plan for us to gracefully handle NASDF? I have the feeling that it takes more than simply:

  • Delete it from Nyxt sources
  • Bump the NASDF version in ntemplate
  • Send a patch to Guix
  • Add as a submodule

I'm very confused about some PRs that seem somewhat related to it.

Also could you please clarify who is working on what, and who is responsible for what? Let's please make use of Github's facilities by assigning issues to people. Thanks.

@aadcg aadcg added the high label May 24, 2023
@Ambrevar
Copy link
Member

Can you give more context? I don't understand the problem you are trying to solve.

In my opinion, there is nothing to be done beside updating NASDF everywhere (which I'll do soon).

While there are 2 PRs for NASDF, which issues are you talking about?

@aadcg
Copy link
Member Author

aadcg commented May 26, 2023

Can you give more context? I don't understand the problem you are trying to solve.

In my understanding, the fact that the Guix package definition for Nyxt requires deleting libraries/nasdf is something that we fix as soon as possible.

(source
     (origin
       (method git-fetch)
       (uri (git-reference
             (url "https://github.com/atlas-engineer/nyxt")
             (commit version)))
       (sha256
        (base32
         "1jzlpi2iam15f0724nh6pb0zfs8d89mrf3zkvd87g99aq9w2h02a"))
       (file-name (git-file-name "nyxt" version))
       (modules '((guix build utils)))
       (snippet
        `(begin
           (delete-file-recursively "libraries/nasdf")
           #t))))

I'm just trying to understand if someone is already handling this transition, and offer my help if needed!

I believe that the necessary steps include:

  • Delete libraries/nasdf from Nyxt sources
  • Bump the NASDF version in ntemplate
  • Send a patch to Guix that updates NASDF
  • Send a patch to Guix that updates Nyxt (delete the snippet that removes libraries/nasdf)
  • Add NASDF as a git submodule

@Ambrevar
Copy link
Member

Ambrevar commented Jun 7, 2023

The thing is that NASDF handles the Git submodules, so we can't ship NASDF as a submodule. Thus we can't remove it from the Nyxt source, unless we find some other means to fetch it.

Ideas welcome!

@aadcg
Copy link
Member Author

aadcg commented Jul 5, 2023

I think the issue here is that if we'd be handling nasdf gracefully, then we wouldn't need to delete it from sources in the Guix package definition (as below).

(snippet `(begin (delete-file-recursively "libraries/nasdf") #t))

Once #3003 is merged, I'll revisit this issue.

@Ambrevar
Copy link
Member

Ambrevar commented Jul 6, 2023

t if we'd be handling nasdf gracefully

What do you mean?

@aadcg
Copy link
Member Author

aadcg commented Sep 11, 2023

We need to urgently fix the issue regarding NASDF. The situation as it stands is cumbersome. We need one single reference of truth and to avoid needing to update all packages that depends on NASDF manually.

@dariof4
Copy link

dariof4 commented Sep 11, 2023

For the nix nyxt package I was using ntemplate's NASDF, but with the current 3.7.0 release it failed to build with ntemplate's NASDF, I needed to swap to nyxt's NASDF to make it work, I agree that having a single source of truth would be nice.

@aadcg
Copy link
Member Author

aadcg commented Sep 11, 2023

@dariof4, yes I'm also mildly infuriated since in order to update the Nyxt package on Guix I need to make a NASDF release. I'll notify you when it's done.

Ultimately, it's my fault too. If I make changes from Nyxt's NASDF, I should have propagated those to ntemplate's NASDF. But, in conclusion, this workflow is deeply flawed.

@aartaka
Copy link
Contributor

aartaka commented Sep 11, 2023

We need one single reference of truth and to avoid needing to update all packages that depends on NASDF manually.

This single reference of truth that was agreed on is ntemplate. So all the changes should first propagate there (even if they are initiated in Nyxt) and then to all the N-libraries and Nyxt itself.

I'm also mildly infuriated since in order to update the Nyxt package on Guix I need to make a NASDF release. I'll notify you when it's done.

This sounds like a regular dependency update, like bumping nfiles to fix a bug or add a new file type. We do update dependencies before a release, which is absolutely normal given their depelopment. Am I missing someting?

this workflow is deeply flawed.

I remember us brainstorming it elsewhere only to conclude that this weird situation is the best option we have. More R&D about how to make it prettier certainly won't hurt, but that's likely to end up deep into the uncharted territories of ASDF.

See https://asdf.common-lisp.dev/#extensions for (some of the) known ASDF extensions. These might have some insights on packaging. But, based on a cursory look I had over these, the process seems to be one of:

  • Either load the library with ASDF extensions (like cffi or LASS), and then, as a separate workflow, define new systems using these library-provided ASDF extensions.
  • Or really bundle the library with the code.

In either case, you cannot treat ASDF extension as a regular library, which effectively results in two layers of end-user library building/loading:

  • First one loads all the ASDF extensions library system uses.
  • And then they load the actual library with all of its dependencies, safely relying on :defsystem-depends-on.

Given that NASDF is mostly Atlas Engineer-specific and is an exclusively ASDF-related library (compared to the end-user valuable cffi and LASS), it is not really comfortable to have it fetched remotely (especially given that NASDF itself usually is in charge of fetching dependency submodules). So it's better to bundle it together with the code.

Another ugly approach would be to inline it inside the .asd file. We used to do that, but such an approach warrants a much higher footgun concentration:

  • Modifying anything in asdf package (which all the .asd files implicitly/explicitly use) is risky because it basically influences every single .asd file in the running Lisp image. Maclisp all over again 🙈
  • This is even easier to get wrong, because one can accidentally make a typo or miss some slight change in the inlined code.
  • And it also makes .asd files harder to read and parse.
    • Given the complexity of NASDF, .asd files inlining it would easily end up with 1K+ LoC.

@aadcg
Copy link
Member Author

aadcg commented Sep 11, 2023

@dariof4 You can use the latest NASDF release. The diff below may help you on NixOS.

 (define-public sbcl-nasdf
-  (let ((commit "dd9fb2df7174464b54561b2a2f3c3e00fdd5d4f7"))
+  (let ((commit "ab7a018f3a67a999c72710644b10b4545130c139"))
     (package
       (name "sbcl-nasdf")
-      (version "0.1.7")
+      (version "0.1.8")
       (source
        (origin
          (method git-fetch)
@@ -25838,7 +25837,7 @@ (define-public sbcl-nasdf
                (commit commit)))
          (file-name (git-file-name "cl-ntemplate" version))
          (sha256
-          (base32 "1q8ky8hz8xrr37h7yyc6ysvrcwlsp1i6r2x44c060drspgjbqj70"))))
+          (base32 "15j7kqxvn0blr0i2xgk0il0ia91p28clfqxdh00vlp423v9a2wbx"))))
       (build-system asdf-build-system/sbcl)

@aadcg
Copy link
Member Author

aadcg commented Sep 11, 2023

This sounds like a regular dependency update, like bumping nfiles to fix a bug or add a new file type. We do update dependencies before a release, which is absolutely normal given their depelopment. Am I missing someting?

You're missing the fact that there's no single point of truth. For the platforms that rely on the git submodules to manage the CL dependencies, all they care about is Nyxt's NASDF version available at libraries. In other words, NASDF (ntemplate) is not part of .gitmodules. On the platforms that don't need the git submodules (such as Guix) we need to delete NASDF from libraries. This suffices to show that the current situation is unmaintainable.

@aartaka
Copy link
Contributor

aartaka commented Sep 11, 2023 via email

@dariof4
Copy link

dariof4 commented Sep 11, 2023

@dariof4 You can use the latest NASDF release. The diff below may help you on NixOS.

 (define-public sbcl-nasdf
-  (let ((commit "dd9fb2df7174464b54561b2a2f3c3e00fdd5d4f7"))
+  (let ((commit "ab7a018f3a67a999c72710644b10b4545130c139"))
     (package
       (name "sbcl-nasdf")
-      (version "0.1.7")
+      (version "0.1.8")
       (source
        (origin
          (method git-fetch)
@@ -25838,7 +25837,7 @@ (define-public sbcl-nasdf
                (commit commit)))
          (file-name (git-file-name "cl-ntemplate" version))
          (sha256
-          (base32 "1q8ky8hz8xrr37h7yyc6ysvrcwlsp1i6r2x44c060drspgjbqj70"))))
+          (base32 "15j7kqxvn0blr0i2xgk0il0ia91p28clfqxdh00vlp423v9a2wbx"))))
       (build-system asdf-build-system/sbcl)

@aadcg Perfect, it works correctly with ntemplate now, thanks! 👍

@jmercouris
Copy link
Member

The idea of copying/pasting code between different projects doesn't sound like a solution to me. Sounds like a very error prone, and bad solution.

We'll have to do some more thinking about this Artyom. I understand your perspective, but I simply disagree.

@aartaka
Copy link
Contributor

aartaka commented Sep 11, 2023 via email

@aadcg
Copy link
Member Author

aadcg commented Nov 13, 2023

I believe that the only sane solution is to have NASDF exclusively on Nyxt's codebase. All of our libraries that rely on it should drop this dependency. It makes little sense to handle a CL library's dependencies via git submodules.

@aadcg aadcg mentioned this issue Nov 27, 2023
7 tasks
@aadcg aadcg self-assigned this Nov 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging a pull request may close this issue.

5 participants