> Maxime Devos writes: > Copyright and license headers are missing. Also, usually we don't do > per-package modules but rather thematic modules, though that's not a hard > rule especially if there are technical problems with that. Ah, right, I forgot about those. Not used to creating new files. Will be in the patch I'm sending shortly. As for the per-package rule, this will not be the only package for this file. Aside from the 2 user-facing packages and 3 bootstrap packages in this series, I'm hoping to package a further (at least) 3 packages—a standard library, a DSL, and a primer (info). I'm also hoping to package an emacs mode, a few fonts, and an update to a few packages to add BQN support. But those have their own files, and won't be included here. Is that enough of a justification for a new file, or should I look to add this to another? I /suppose/ apl.scm would work, though technically that would be like scheme, common lisp, arc, clojure, et al being grouped in the same language. WDYT? > I don't think you are using (guix deprecation) below? I don't think I am either, now that You mention it. Somehow that made its way into my starting template; I will remove it in the forthcoming patch. > I have looked at the 'v0.2.1' tag, and it points at the 0bbe096... commit, so > you are actually packaging version 0.2.1, not some git commit after > v0.2.1. As such, no need for (git-version ...), you can just write "0.2.1" > there. Then 'revision' becomes unused and can be dropped, and 'commit' is > only used in a single place anymore so it can be inlined. This honestly confused me a bit at first, but I think I see what You mean now. Since it's an official upstream version, that can be the version, instead of using git-version. I will apply these changes as well. >> + (native-inputs (list `(,icedtea-8 "jdk") coreutils zip)) > coreutils is an implicit (native-)input, likely no need to to mention it. > > Also, the Makefile mentions that the executable to start things has > #!/bin/bash -- to properly patch is when cross-compiling, you need > 'bash-minimal' or 'bash' in inputs, otherwise it will be patched for the > wrong architecture. > > Also, that script runs 'java' -- make sure it is patched too such that java > will actually be found -- and to patch it, you need to have icedtea:out or > openjdk:out in 'inputs'. Per the feedback I received from Liliana, I will use icedtea:out to (as there is less bootstrapping required if building everything from source). I'll also add bash-minimal in place of the unneeded coreutils, and place both icedtea:out and bash-minimal in inputs rather than native, as they are expected for the target machine. >> + (outputs '("out")) > That's the default, no need to set it. >> + (list #:imported-modules `(,@%gnu-build-system-modules (guix build >> + syscalls) > For formatting, (guix build syscalls) should be on a separate line. >> + (synopsis "BQN implementation based on dzaima/APL") >> + (description "BQN implementation based on dzaima/APL.") > The synopsis and description are identical, and this doesn't explain much to > people who don't know what 'BQN' is. Can it be rewritten such that people not > familiar with BQN can decide whether this ‘BQN’ is something that's useful > for them? '(guix)Synopses and Descriptions' has more information. >> + (lambda* (:#key tests? >> + #:allow-other-tags) > Why the : before #:key? Also, no need to break it in separate lines. All of these points are addressed in the forthcoming (v4) patch. In particular, thanks for calling out the description: It had been a placeholder while I was getting things working, and I'm happy to replace it with something more descriptive. And the `:#key` was a typo, unsure how it has worked thus far, as I had tests disabled up until recently. Maybe I was using the stock check phase, then. I forget, tbh. >> + (add-after 'install 'reorder-jar-content >> + (lambda* (#:key outputs #:allow-other-keys) >> + (apply (assoc-ref ant:%standard-phases >> + 'reorder-jar-content) >> + #:outputs (list outputs)))) > 'output's is a list of strings, now you are giving reorder-jar-content a list > of lists of strings > However, looking at (guix build ant-build-system), it looks like it just > wants a list of strings. > > As such, maybe it should be: > > (add-after 'install 'reorder-jar-content >   (assoc-ref ant:%standard-phases 'reorder-jar-content)) > > ? (untested)  Possibly likewise for the other phases. This intrigues me. The list of outputs was a kludge to allow the function to accept the singleton output. If the singleton is still wrapped in a list, then I'm unsure why it fails. Perhaps I need to test this more; will do so before sending v4. >> (the implementation is even under single-license GPL!) [...] > You are writing license:expat in the 'license' field, not > license:gplN/license:gplN+. Is it Expat or is it GPL? So, this package (dbqn) is expat. The implementation I referenced above (which is recommended and actively developed) is a different one (cbqn) that relies on this one as a dependency for bootstrapping, and is under gpl. I really just wanted to add cbqn to guix, but can't do so without dbqn, which /is/ still functional, is depended on by a bunch of tooling for the language, and may as well also be a package because of the above (sort of like how someone using common lisp may view clisp as a slow dependency of sbcl, or they may instead choose to use it as their target implementation). >> + (synopsis "Official BQN sources in BQN") > If they are just sources, you can do (define bqn-bytecode-sources (origin > ...)) I had not realized the package was unneeded! That might simplify things greatly. I love packaging things for guix; I always learn something new. >> + (home-page"https://github.com/mlochbaum/BQN.git") >> + (license license:gpl3)))) > The 'LICENSE' file says something different. I don't think that's the home > page, maybe instead? So, is the homepage of the language, but the sources we're using are only a part of that language definition. In particular, we're only using the src/ and test/ directories of that repository. We're ignoring the docs, commentary, editor plugins, fonts, implementation instructions, spec, tutorial, javascript implementation, etc. We really are just targetting the sources for the bytecode and the tests, which is why I thought the repo itself makes a better homepage. Should I change that to the homepage for the language? As for the LICENSE file, indeed, it is isc. Thanks for the catch. > >> + (chmod "BQN" 493) > Hexadecimal would be clearer. As in Your second message, I will use the octal notation per Liliana. >> + "The expected implementation for the BQN language, >> +according to the official documentation of that specification.") > expected -> standard (what's 'expected' depends on the user, they might want > a different implementation), unless it's not the standard implementation. > > 'documentation of that specification' -> 'the specification (pleonasm) > > And maybe remove 'official', given the absence of 'official' in the > descriptions of, say, guile, gcc, openjdk and java, this sounds marketing and > unfair to me. I will change this to "The standard implementation of the BQN language, according to the specification." (Also, thank You for teaching me 'pleonasm'!) > (singeli-bootstrap:) > >> + (let* ((tag "0") >> + (revision "1") >> + (commit "fd17b144483549dbd2bcf23e3a37a09219171a99") >> + (hash "1rr4l7ijzcg25n2igi1mzya6qllh5wsrf3m5i429rlgwv1fwvfji") >> + (version (git-version tag revision commit))) > I'm not seeing a '0' tag anywhere in the repository -- I dont see any tags at > all tere. There are none; I thought the absence of any tage necessitates a '0' for the version number, and wanted to keep the definitions standardized. Should I inline the 0 instead in the git-version? >> + (inputs (list bqn-bytecode-sources libffi singeli-bootstrap)))) > For cross-compilation, I would have expected sengili-bootstrap in > native-inputs, not inputs, assuming that it is used as a compiler. Does > cross-compilation (with --target) work? Singeli is written in BQN, which is an interpreted language. So, as long as the BQN interpreter is for the correct architecture, it will work. It will never be compiled itelf. Thank You for all of the feedback! I'll try to have a revised set of patches in by this evening EDT, in the next 8 hours. -- Christopher Rodriguez