Remove an undeclared ns cycle

[arohner]

This resolves an issue AOT'ing when using https://github.com/griffinbank/rules_clojure

[arohner]

Sorry, I'll remove the griffinbank changes

[arnaudgeiser]

This change could go for me (modulo the griffinbank removal).

Can I also ask you for a comparison between the behaviour you had before and the one you have now? (on the PR description - the native-image invocation).

[KingMob]

Zach did some funky stuff like mid-file requires to avoid some circular dependency design issues (and just because it was the dawn of Clojure, before people realized this was more trouble than it was worth). I always meant to fix it, but never got around to it, so thanks for tackling this!

So, a few notes:

• How does this not break the call to manifold.stream.graph/connect in the connect defn, and the BufferedStream deftype+?
• If you use requiring-resolve, be sure to check and update the deps to specify a min version of Clojure now. I don't recall if there's a way to do that in Lein, but at the least, you should add something in the README about the min Clj version being 1.10 now.
• If you're seeking AOT compilation, you should know Manifold and Aleph aren't especially amenable to that, primarily because of stuff like this.

[arohner]

Some context:

We are compiling our project using Bazel + https://github.com/griffinbank/rules_clojure. This makes AOT incremental, per-ns, rather than the compiling the whole project in a single pass using lein or tools.build. This means each ns produces a separate jar containing only .class files from that ns, and uberjars consume all of the AOT ns jars rather than source files. The upside is that compilation is parallelizable and incremental (if you uberjar, touch a source file and then uberjar again only that file and nses that depend on it recompile).

The tradeoff is that ns declarations must be correct, because under bazel, you must declare "target A depends on source files B and C". Bazel uses a sandbox to enforce this, so If your dependencies aren't properly declared, compilation will fail.

With the patch in place, our project AOT compiles just fine (and our code that uses caesium passes). We're using caesium -> byte-streams -> manifold.

Why it broke: if you start compiling from manifold.stream.core, there's a reference to manifold.stream.graph/downstream which is not declared in the ns block.

With the patch in place, I can annotate the AOT manifold.stream.core jar to have a runtime dependency on manifold.stream.graph. Without it, manifold.stream.core has a compile-time dependency on manifold.stream.graph (because it refers to downstream), but manifold.stream.graph has a compile time dependency on manifold.stream.core (because it appears in the :requires) block.

How does this not break the call to manifold.stream.graph/connect in the connect defn, and the BufferedStream deftype+?

Good catch. I don't know, but I see a few options:

• The tests all pass, so I got lucky in that something "always" calls downstream first
• the tests are incomplete

I've made the same change in those two other places, and verified there are no other calls to manifold.stream.graph without a require or requiring-resolve.

If you use requiring-resolve, be sure to check and update the deps to specify a min version of Clojure now. I don't recall if there's a way to do that in Lein, but at the least, you should add something in the README about the min Clj version being 1.10 now.

Good idea. The project.clj already has [org.clojure/clojure "1.11.1" :scope "provided"], but I've added it to the CHANGES.md file because there didn't seem to be a good location in README.

[KingMob]

Go ahead and add the notice about the clj version to the Readme somewhere and leave CHANGES untouched. Not enough people read the changelog, and this is a breaking change.

I'm still a little concerned that it passed the tests when you removed the ns, but before you added the requiring-resolve.

Have you tried this with a standard lein built setup? Most people aren't using Basel.

[KingMob]

• The tests all pass, so I got lucky in that something "always" calls downstream first
• the tests are incomplete

If we've just been luck so far, we shouldn't rely on it. One way to check is to fire up a new REPL, load just that namespace first, and exercise that code. That minimizes any chance of something requiring it for us.

If the tests are incomplete, can you add a test that fails before, and succeeds after, your change?

[arohner]

How does this not break the call to manifold.stream.graph/connect in the connect defn, and the BufferedStream deftype+?

I figured this out. If we start from manifold.stream (the ns with a call to manifold.stream.graph, which "should" break), look at the ns declaration. it contains:

  (:require 
...
    [manifold.stream
      [core :as core]
    ...
     deferred]

m.stream.core used to call m.stream.graph in the middle of the file, and no longer does until downstream is called. But manifold.stream.deferred does depend on manifold.stream.graph, so it's always loaded before the manifold.stream ns block finishes. There isn't a bug, so a test wouldn't show anything. A more ideal fix is to refactor these cycles away entirely, so requiring resolve isn't necessary.

I've updated the README now. I believe that addresses all comments.

[KingMob]

Hmmm, this is a little suspicious.

It looks like it can be safely removed, but it used to be the case that some namespaces were required to force initialization of necessary data structures that were indirectly needed. Yet another reason indirect requires are a bad idea.

Removing this looks ok afaict, tho.

[KingMob]

@arohner Thanks for digging into this.

I'm glad it works, but it's never ideal to rely on indirect requires if we're directly invoking from a namespace we depend on. That's always a time bomb waiting to happen. Instead, can you add manifold.stream.graph directly in the requires without breaking anything?

If so, we should be able to skip the requiring-resolve. That's really designed for optional, conditional, and REPL requires. If the requiring-resolve should succeed 100% of the time in normal code, it should be replaced with a plain require.

---

Also, please remove the reference to the version number in the README. We'll add it when we cut a release. And given the breaking change, we may want to bump it up to 0.5.x or even 1.0.0.

[KingMob]

Change this from 0.4.4 to "Unreleased changes"