Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make the prefix a bit more unique #6

Closed
expez opened this issue Jun 12, 2015 · 9 comments
Closed

Make the prefix a bit more unique #6

expez opened this issue Jun 12, 2015 · 9 comments

Comments

@expez
Copy link
Contributor

expez commented Jun 12, 2015

As discussed here

In the referenced thread a desire to prefix the dependencies with the project name is expressed. I think this is a mistake. We want to eliminate dependency clashes, when several versions of a class is loaded into the classloader, but we don't want to load multiple versions of identical code under different names. If we use the project name as a prefix we won't be able to share the common code and the run time will grow unnecessarily large.

@frenchy64
Copy link

This seems like a reasonable default. I would like to use mranderson to generate a flat folder of dependencies and I've hacked it to do so in my fork. Customising the prefix would be a nice option.

@expez
Copy link
Contributor Author

expez commented Jul 5, 2015

I really think it's a mistake to use a new prefix for each project @benedekfazekas. Using a unique prefix that we can easily filter on when needed is great, but you're now loading n versions of the same thing into the runtime for no reason.

@benedekfazekas
Copy link
Owner

not sure I follow. this commit only replaces "deps" with projectname-projectversion (as default) or your custom prefix as the first element of rewritten namespaces. in fact "deps" used to be repeated for transient dependencies but the customisable prefix is not repeated any more. so it used to be this deps.cheshire.v5v4v0.deps.tigris... not it is foobar.cheshire.v5v4v0.tigris...

can you post an example tree what you would like to see perhaps?

@expez
Copy link
Contributor Author

expez commented Jul 6, 2015

Here's a concrete example:

cider-nrepl and refactor-nrepl have these deps in common at present: java.classpath, tools.reader and tools.namespace.

With this new change these libraries will be loaded into the runtime as cider_nrepl.tools.reader.v.0.9.2... and as refactor_nrepl.tools.reader.v0.9.2.... We've thus aliased the same thing, for no reason, and this causes our runtime to grow larger than it needs.

What I would've liked to have seen is this:

mranderson.v.4.2.tools.reader.v.0.9.2.... This prefix has the following benefits:

  • It's unique enough that we can filter out rewritten deps in tooling
  • It allows sharing of code
  • It now includes the version of mranderson used to inline the deps which ensures the shared code is in fact identical.
  • It doesn't burden the user with a setting which is in fact doing them more harm than good.

@benedekfazekas
Copy link
Owner

I see and appreciate your point. a few points:

  1. you can already achieve this by defining an appropriate custom prefix (eg mrandersonv0v4v4 or similar)
  2. I could even make this default I guess...
  3. the same would be achieved if/when we merge refactor-nrepl and cider-nrepl ;)
  4. I absolutely see the practical benefit of this I am just not sure as mranderson's goal is to separate dependencies. with this approach we would again let dependencies 'merge' (although only if they are the same version: true) across projects... in other words: I am not sure that however important load time is it should be mranderson's concern... (even if this sounds silly)

any other opinion on this? @frenchy64 @rundis @bbatsov?

@expez
Copy link
Contributor Author

expez commented Jul 6, 2015

  1. you can already achieve this by defining an appropriate custom prefix (eg mrandersonv0v4v4 or similar)

If mranderson sees wider adoption this is going to be impossible in practice because it would require coordination between library authors. How can you coordinate with authors of libraries you're unaware of?

the same would be achieved if/when we merge refactor-nrepl and cider-nrepl ;)

Yes, but there's already interest from other projects in using mranderson....

I am not sure that however important load time is it should be mranderson's concern

It's not just the load-time, it's also runtime performance (the JIT thinks the code is different when it's the same) and the size of the runtime (same thing loaded by different names).

There isn't even a tradeoff here that I can see, all the benefits are on the side of sticking to a single, version-specific, prefix for rewriting.

@benedekfazekas
Copy link
Owner

still interested in other ppls opinion but i am nearly sold to change the default just to mranderson (sans version that is) -- best self marketing for the project too i guess... ;)

@benedekfazekas benedekfazekas reopened this Jul 6, 2015
@expez
Copy link
Contributor Author

expez commented Jul 6, 2015

Without the version, or some other uniquifyer, you might get conflicts if mranderson starts publishing clojure artifacts. In a sense it has the same problem as the deps prefix had. So at least make it some variant of mranderson.deps or mranderson.inlined_deps.

You also risk claiming that rewritten deps are identical when they might not be, because they were created by different versions of mranderson. I'm not sure what sort of problems this might cause, which makes me want to avoid the situation altogether and stick with a version prefix, even if that leads to slightly less sharing.

@frenchy64
Copy link

I don't fully understand the problems related to having multiple copies of
the same namespace in the classpath, though I understand only one will load.

Is a better solution a package manager? ie. rebundling jars with mranderson
prefixes. This way if two libraries depend on
[com.mranderson/tools.namespace-zero-six-four "1.0.0"], it is really just
one copy of the source/AOT.

Perhaps a cron job can watch repositories and automatically copy/release
them.

Slight crazy I realise, but more comfortable than having 20 copies of the
same namespace in my classpath.

On Mon, Jul 6, 2015 at 6:08 PM, Lars Andersen [email protected]
wrote:

Without the version, or some other uniquifyer, you might get conflicts if
mranderson starts publishing clojure artifacts. In a sense it has the same
problem as the deps prefix had. So at least make it some variant of
mranderson.deps or mranderson.inlined_deps.

You also risk claiming that rewritten deps are identical when they might
not be, because they were created by different versions of mranderson. I'm
not sure what sort of problems this might cause, which makes me want to
avoid the situation altogether and stick with a version prefix even if that
leads to slightly less sharing.


Reply to this email directly or view it on GitHub.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants