Static link file contains bogus unit names

[sjamaan]

If you perform chicken-install srfi-41 and inspect the generated streams.derived.link file, it contains srfi-9 as a dependency, which is incorrect because srfi-9 is a primitive module which is part of libchicken, so no static linking is needed.

This breaks anything that relies on srfi-41 and tries to link statically, for example ​https://github.com/markjfisher/aoc-chicken/blob/master/build/static-build.sh

This seems to be a regression from 5.2.0 (and therefore a definite blocker for 5.3.0), because there the generated link file contains only (streams.primitive type-checks), which is correct.

I haven't done any serious investigation yet, but likely candidates for this regression are 3fd42518 and 41a1decf.

[felix winkelmann]

The problem seems to be caused by 41a1decf, indeed. Requirements that are among builtin-features are handled differently now and end up in required-libraries (the chicken-load#core-unit? test doesn't filter these out). The only fix I can think of is to add another explicit filtering out of builtin-features when populating the .link file. Unfortunately builtin-features is only visible in eval.scm.

That this problem appears in streams.derived is merely caused by it importing srfi-9.

[sjamaan]

The only fix I can think of is to add another explicit filtering out of builtin-features when populating the .link file. Unfortunately builtin-features is only visible in eval.scm.

I think a simple fix could be to partially restore the original implementation of ##sys#process-require, particularly the mark-static argument. This could then mutate required-libraries, and drop the (set! required-libraries (lset-adjoin/eq? required-libraries lib)). I think the set! here is the incorrect bit, as it doesn't have enough information here to know whether it's a library that should be required; that, only process-require knows.

A more elaborate but arguably cleaner fix would involve adding a predicate to eval.scm check whether it's part of core-unit-requirements or builtin-features or core-units, and use that as a guard around that set! mentioned above. That does have a drawback though: there's repeated code so it might possibly diverge over time resulting in new bugs of this kind.

[evhan]

Built-in libraries are already filtered out from the set of required libraries when generating the link file in batch-driver.scm, using chicken.load#core-unit?. It seems that may simply not go far enough, and it may be enough to add builtin-features to that check (haven't been able to test yet though).

To be clear though I don't think it's a problem that the identifier is ending up in required-libraries, only that it's being included in the link file. That list collects everything for which a ##core#require is generated, and any smarts (noops/rewriting/etc.) is applied later.

[evhan]

See attached patch (again, untested, but I think this is a good direction).

[felix winkelmann]

Thanks, Evhan. But I think required-libraries has a clear meaning and only things should appear in this list that are both truly required and libraries. ##sys#process-require is the right place to make the distinction.

[evhan]

Thanks, Evhan. But I think required-libraries has a clear meaning and only things should appear in this list that are both truly required and libraries. ##sys#process-require is the right place to make the distinction.

My thinking was that it would include "libraries that have been required", as in, "libraries for which a ##core#require form has been processed", similar to how linked-libraries contains "libraries that have been linked" or used-units contains "units that have been used". Although there isn't any filtering done for those lists, so they aren't really a useful comparison in that way.

Anyway, if the filtering is moved then I think chicken.load#core-unit? should go away completely, since this is the only reason it exists. I just wanted to mention that when Peter described the solution below, it is something that already exists and just needs extension:

A more elaborate but arguably cleaner fix would involve adding a predicate to eval.scm check whether it's part of core-unit-requirements or builtin-features or core-units, and use that as a guard around that set! mentioned above.

[felix winkelmann]

I can't quite see how Peter's proposed attempt is cleaner. There is now one spot, ##sys#process-require, which, well, processes the require form and decides on both the expansion and whether it is an externally loadable/linkable module. So no filtering is required, as we try to build the requirements-list properly from the start.

But if you have a better proposal, go on and provide a patch. I find the posted solution fine, but have no problem with a different approach, if others find it to be an improvement.

[sjamaan]

Patch is fine, applied as a82aa3b2 / 5ea914a8

[felix winkelmann]

Unfortunately internal modules like chicken.foreign and chicken.csi still leak out, One is caused by a wrong result for members of core-unit-requirements, the other case needs to be investigated.

[Mario Domenech Goulart]

The patch to address the chicken.foreign issue, submitted by Felix to chicken-hackers (​https://lists.nongnu.org/archive/html/chicken-hackers/2021-10/msg00001.html) has been pushed to both master (f98e4a20b3bff21925db60673f2c46f264cfb442) and prerelease (3b5d5039ad54f959bd8b88d96d90eff18107d0b0).

[felix winkelmann]

Fixed by f98e4a20 and improved by de25570c, da35e45b and 137ae4ad.