Summary

Static link file contains bogus unit names

Metadata

Attachments

Description

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`.

Changes and comments

[2021-09-24 22:17:50 UTC] felix changed component from build system to compiler

[2021-09-24 22:17:50 UTC] felix wrote:

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`.

[2021-09-24 22:17:50 UTC] felix wrote:

1632514700988987

[2021-09-24 22:17:50 UTC] felix wrote:

1632515090217063

[2021-09-27 09:58:24 UTC] sjamaan wrote:

> 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.

[2021-09-27 22:17:22 UTC] evhan wrote:

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.

[2021-09-27 22:17:22 UTC] evhan wrote:

1632774175753200

[2021-09-27 22:28:53 UTC] evhan wrote:

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

[2021-09-28 12:53:17 UTC] felix set owner to felix

[2021-09-28 12:53:17 UTC] felix wrote:

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.

[2021-09-28 12:53:17 UTC] felix changed status from new to accepted

[2021-09-28 22:02:08 UTC] evhan wrote:

> 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.

[2021-09-29 19:01:43 UTC] felix wrote:

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.

[2021-10-06 07:29:12 UTC] sjamaan changed status from accepted to closed

[2021-10-06 07:29:12 UTC] sjamaan set resolution to fixed

[2021-10-06 07:29:12 UTC] sjamaan wrote:

Patch is fine, applied as a82aa3b2 / 5ea914a8

[2021-10-07 00:08:27 UTC] felix changed status from closed to reopened

[2021-10-07 00:08:27 UTC] felix removed resolution fixed

[2021-10-07 00:08:27 UTC] felix wrote:

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.

[2021-10-07 00:08:27 UTC] felix wrote:

1633558130495658

[2021-10-07 21:41:14 UTC] mario wrote:

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).

[2021-10-22 13:03:21 UTC] felix changed status from reopened to closed

[2021-10-22 13:03:21 UTC] felix set resolution to fixed

[2021-10-22 13:03:21 UTC] felix wrote:

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