Undocumented features dependency

Bug #518467 reported by Robert P. Goldman
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
ASDF
Fix Released
Medium
Faré

Bug Description

In the code for traverse, there's a THREE-argument form for a FEATURE dependency:

((and (string-equal
         (symbol-name (first d))
         "FEATURE")
        (find (second d) *features*
              :test 'string-equal))
   (appendf
    forced
    (do-one-dep op (second d) (third d))))

What this is doing is handling a requirement of this form:

(:feature component-and-feature-name [version])

And the semantics seems to be that if COMPONENT-AND-FEATURE-NAME is in the *FEATURES* list, then we should try to load it (using optional version information).

I note that this is nowhere documented. and wonder where it came from. I had a first shot at tracking it down with blame, but got lost in whitespace changes. Perhaps someone else can figure this out.

Should it be documented or should it be excised?

Changed in asdf:
importance: Undecided → Medium
milestone: none → version2
Revision history for this message
Robert P. Goldman (rpgoldman) wrote :

OK, I would really like to see this killed. There's no evidence that anyone uses it, and it seems to have just crept in somewhat randomly.

This comment is to put the ASDF community on notice that I intend to rip this out. If you object, sing out here.

Changed in asdf:
assignee: nobody → Robert P. Goldman (rpgoldman)
Revision history for this message
Faré (fahree) wrote :

_3b` tells me that at least nikodemus uses it in sb-cga:

http://github.com/nikodemus/sb-cga/blob/master/sb-cga.asd

(:module "ports"
            :if-component-dep-fails :try-next
            :components ((:file "sbcl" :in-order-to ((compile-op (feature :sbcl))))
                         (:file "ccl" :in-order-to ((compile-op (feature :ccl))))))

We should probably document that, and include a test case in our test suite.

As long as there is no regression in support or (lack of) documentation, I'd like to make that a 2.1 milestone feature so it doesn't impede the ASDF 2 release...

Revision history for this message
Robert P. Goldman (rpgoldman) wrote :

Are you sure that this is the answer, Faré?

Won't the example from sb-cga.asd be handled by the first piece of the COND:

               (cond ((eq op 'feature)
                      (or (member (car dep) *features*)
                          (error 'missing-dependency
                                 :required-by c
                                 :requires (car dep))))
                     (t

rather than in the second:

                     (t
                      (dolist (d dep)
                        ;; structured dependencies --- this parses keywords
                        ;; the keywords could be broken out and cleanly (extensibly)
                        ;; processed by EQL methods, but for the pervasive side-effecting
                        ;; onto FORCED
                        (cond ((consp d)
                               (cond ((string-equal
                                       (symbol-name (first d))
                                       "VERSION")
                                      (appendf
                                       forced
                                       (do-one-dep op (second d) (third d))))
                                     ;; this particular subform is not documented, indeed
                                     ;; clashes with the documentation, since it assumes a
                                     ;; third component
HERE ((and (string-equal
                                            (symbol-name (first d))
                                            "FEATURE")
                                           (find (second d) *features*
                                                 :test 'string-equal))
                                      (appendf
                                       forced
                                       (do-one-dep op (second d) (third d))))
                                     (t
                                      (error "Bad dependency ~a. Dependencies must be (:version <version>), (:feature <feature> [version]), or a name" d))))
                              (t
                               (appendf forced (do-one-dep op d nil)))))))))

It's this second bit that I don't know what to make of.

The first piece is what nikodemus is using, as far as I can tell. But since I don't understand that second use of feature AT ALL, I could be wrong.

This code is DEEPLY mysterious to me --- note that the two uses of FEATURE also differ --- the second use will match a keyword while the former will not (unless a keyword is "de-keyworded" upstream).

I would be very grateful if someone who understood the definition of DO-DEP could rewrite it to make its meaning clear....

Revision history for this message
Faré (fahree) wrote :

git blame -w asdf.lisp tells me it comes from Gary King's commit e7e46819.
It looks like it's modelled after VERSION, but erroneously takes three arguments instead of four, and/or
erroneously copy-pasted the call
(do-one-dep op (second d) (third d))
instead of
(do-one-dep op (third d) nil)

Obviously, nobody ever used it.
I suppose it can be ditched, or it could be fixed and documented.

Does anyone use the related VERSION thingie?

Revision history for this message
Faré (fahree) wrote :

Downgrading to version 2.1, since lack of documentation is no regression as compared to what was before.

Also, I'll commit the
(do-one-dep op (third d) nil)
modification so at least it works somewhat.

Changed in asdf:
milestone: version2 → version2.1
Revision history for this message
Faré (fahree) wrote :

OK, so there were two different :feature features, none documented, and only one semi-working, the one used by Nikodemus. I just removed it, because it was seriously cheating with the dependency graph and got in the way of a major simplification I did in 2.26.21.

The other one has been present for quite some time, but never working. Indeed, I after fixing a bug in it, I put it behind a cerror telling people to contact me if they saw the message, and no one did. Actually, now that I think of it more, even if I remove the cerror, it doesn't make sense (it allows for conditional dependency, not conditional modules), so I'll delete it.

Closing as WONTFIX.

The real solution is adding an extra slot to components for a feature expression to check before actually including the component in the object graph.

I added just that in 2.26.22.

A component can have an arbitrary :if-feature (:or :some-feature (:not :other-feature)) feature expression among its initargs, and the component will be included in the build graph if and only if the feature expression is true.

Now to document it in asdf.texinfo.

Changed in asdf:
status: New → Won't Fix
Revision history for this message
Faré (fahree) wrote :

OK, I committed some minimal :if-feature documentation.

Changed in asdf:
assignee: Robert P. Goldman (rpgoldman) → Faré (fahree)
status: Won't Fix → Fix Committed
Revision history for this message
Faré (fahree) wrote :

Released in 2.27

Changed in asdf:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.