Skip to content
This repository has been archived by the owner on Apr 13, 2023. It is now read-only.

Overriding in module descriptors #5955

Open
tombentley opened this issue Jan 28, 2016 · 46 comments
Open

Overriding in module descriptors #5955

tombentley opened this issue Jan 28, 2016 · 46 comments

Comments

@tombentley
Copy link

Following an agreeable discussion about module descriptors and assemblies, we decided that we need to allow overrides in module.ceylon. Merging these overrides would apply downwards in a module dependency tree (cycles are not a problem because they always have to be compiled together anyway).

For flatclasspath we would still require additional overrides (to resolve conflicts between non-shared but duplicated dependencies).

We'd need a compile-time version validation phase (to identify diamonds), which would benefit from being able to obtain dependency information without downloading the entire module. This could be a special API for herd, or a separate artifact obtainable separately from the module itself.

@FroMage FroMage modified the milestone: 1.2.2 Feb 9, 2016
@FroMage FroMage modified the milestones: 1.2.2, 1.2.3 Feb 18, 2016
@gavinking
Copy link
Contributor

Here's a proposal for the syntax of this, based on a suggestion by @tombentley:

native("jvm")
module com.my.app "1.0.0" {
    import "org.hibernate:hibernate-entitymanager" "5.0.4.Final" {
        ~ shared import "org.hibernate:hibernate-core" {
            + shared import "javax.transaction:jta" "1.1";
        }
        ~ shared import "org.javassist:javassist";
        ~ shared import "org.hibernate.javax.persistence:hibernate-jpa-2.1-api";
    }
    import "org.hsqldb:hsqldb" "2.3.1";
}

There are three operators:

  • ~ overrides an existing import specified in the module descriptor of a module the current module is importing
  • + adds a new import to a module
  • - removes an import of a module

The + and - operators work basically just like <add> and <remove> in overrides.xml. The ~ operator does the work of <set>, <share>, and <artifact>.

An interesting question is: is it better to write the above like this, with just one level of nesting allowed?

native("jvm")
module com.my.app "1.0.0" {
    import "org.hibernate:hibernate-entitymanager" "5.0.4.Final" {
        ~ shared import "org.hibernate:hibernate-core";
        ~ shared import "org.javassist:javassist";
        ~ shared import "org.hibernate.javax.persistence:hibernate-jpa-2.1-api";
    }
    ~ import "org.hibernate:hibernate-core" {
        + shared import "javax.transaction:jta" "1.1";
    }
    import "org.hsqldb:hsqldb" "2.3.1";
}

That is, can I use one ~ import to adjust the transitivity of the import, and a second one to add a nested import. That is the way we do things with overrides.xml, and I guess it makes more sense, but it confuses the semantics of ~. Perhaps we need a fourth operator.

@gavinking
Copy link
Contributor

We need some way to distinguish "global" overrides from overrides that apply to a single nested import, I suppose. It's not clear to me that the nesting level alone really captures that.

@bjansen
Copy link
Contributor

bjansen commented Mar 7, 2016

Some people are going to complain that ~ is not easy on their keyboard ;)

(FTR it's not even displayed on Mac azerty keyboards, it's a tricky alt+n+space).

@gavinking
Copy link
Contributor

Upon reflection, it seems to me that we should clearly separate regular module imports, and module overrides, since the former only to the current module, and the latter apply recursively to all its dependencies. So we don't want to confuse people about the semantic difference between them. Given that, perhaps a layout like the following is better:

native("jvm")
module com.my.app "1.0.0" {
    import "org.hibernate:hibernate-entitymanager" "5.0.4.Final";
    import "org.hsqldb:hsqldb" "2.3.1";
    module "org.hibernate:hibernate-entitymanager" "5.0.4.Final" {
        ~ shared import "org.hibernate:hibernate-core";
        ~ shared import "org.javassist:javassist";
        ~ shared import "org.hibernate.javax.persistence:hibernate-jpa-2.1-api";
    }
    module "org.hibernate:hibernate-core" {
        + shared import "javax.transaction:jta" "1.1";
    }
}

This is, I suppose, a lot more consistent with how overrides.xml works today.

@gavinking
Copy link
Contributor

Some people are going to complain that ~ is not easy on their keyboard ;)

Well, TBH, I would be perfectly happy using:

  • the actual annotation instead of ~ for import version overrides, and
  • the shared annotation instead of ~ for import export overrides, and
  • nothing instead of + for adding imports.

But then I wouldn't know what to do for removing imports, which @FroMage insists is a necessary capability.

FTR, it would look like this:

native("jvm")
module com.my.app "1.0.0" {
    import "org.hibernate:hibernate-entitymanager" "5.0.4.Final";
    import "org.hsqldb:hsqldb" "2.3.1";
    module "org.hibernate:hibernate-entitymanager" "5.0.4.Final" {
        shared import "org.hibernate:hibernate-core";
        shared import "org.javassist:javassist";
        shared import "org.hibernate.javax.persistence:hibernate-jpa-2.1-api";
    }
    module "org.hibernate:hibernate-core" {
        shared import "javax.transaction:jta" "1.1";
    }
}

The above is rather clean and readable, and more ceylonic (no cryptic symbols). But how would you indicate removal of a dependency?

@quintesse
Copy link
Contributor

nothing instead of + for adding imports.

So how do you distinguish between shared and non-shared additional imports?

But how would you indicate removal of a dependency?

Using deprecated perhaps?

@jvasileff
Copy link
Contributor

the latter apply recursively to all its dependencies

I'm not sure I understand. If module M imports modules A and B/x, and module A imports B/y, is it not possible to change M's import to B/z without affecting A's import? Or, add/remove/change B/x's imports without affecting B/y's imports?

(x, y, and z are versions)

@gavinking
Copy link
Contributor

So how do you distinguish between shared and non-shared additional imports?

With the shared annotation, of course. But additional imports are syntactically different; they include a version number.

@gavinking
Copy link
Contributor

If module M imports modules A and B/x, and module A imports B/y, is it not possible to change M's import to B/z without affecting A's import?

It is possible, yes.

@someth2say
Copy link
Contributor

+1 for actual and shared. Using cryptic symbols like ~ is one of reasons I flee from Scala!
I can't see a reason for removing dependencies, but if we using adjectives, I will propose unshared.

Another point from your proposal, Gavin, is that I feel it duplicating. Assuming there is no reason for overriding modules not being imported (directly or indirectly), why don't place overrides inside module import?

native("jvm")
module com.my.app "1.0.0" {
    import "org.hibernate:hibernate-entitymanager" "5.0.4.Final"  {
        shared import "org.hibernate:hibernate-core" {
                shared import "javax.transaction:jta" "1.1";
            };
        shared import "org.javassist:javassist";
        shared import "org.hibernate.javax.persistence:hibernate-jpa-2.1-api";
    };
    import "org.hsqldb:hsqldb" "2.3.1";
}

@FroMage
Copy link
Contributor

FroMage commented Mar 8, 2016

Yeah, I'm not convinced by symbols. I'd rather have annotations. We can just add a remove annotation, no?

@gavinking
Copy link
Contributor

Another point from your proposal, Gavin, is that I feel it duplicating. Assuming there is no reason for overriding modules not being imported (directly or indirectly), why don't place overrides inside module import?

Well the issue is that:

  1. a module can be buried quite deep in the graph, and
  2. sometimes we need to override something in a definition of a module that is imported in multiple places in the graph.

So I think the singly-nested module elements are the right way to do this.

@gavinking
Copy link
Contributor

We can just add a remove annotation, no?

Feels wrong. Grumble.

@ePaul
Copy link
Contributor

ePaul commented Mar 8, 2016

The other annotations are adjectives, so removed?

@lucaswerkmeister
Copy link
Contributor

dont import "this.module" ;)

@lucono
Copy link

lucono commented Mar 8, 2016

ignore, which is an existing annotation and could kind of be appropriate for this case, but then it's defined in the ceylon.test module.

@lucono
Copy link

lucono commented Mar 8, 2016

Would look something like:

native("jvm")
module com.my.app "1.0.0" {
    import "org.hibernate:hibernate-entitymanager" "5.0.4.Final";
    import "org.hsqldb:hsqldb" "2.3.1";
    module "org.hibernate:hibernate-entitymanager" "5.0.4.Final" {
        shared import "org.hibernate:hibernate-core";
        ignore import "org.hibernate.javax.persistence:hibernate-jpa-2.1-api";
    }
}

@lucono
Copy link

lucono commented Mar 14, 2016

Perhaps in addition to the proposal in this issue, it would be useful to also have a way of optionally indicating that all transitive dependencies of a module import should be themselves treated as directly imported by the module.

Otherwise, developers having this need would still need tools like the ceylon-gradle-plugin to auto-generate module descriptors which surface transitive dependencies of module imports to become top-level module imports themselves.

@gavinking
Copy link
Contributor

gavinking commented Jul 24, 2017

I just made the following work, on branch 5955:

native("jvm")
module jaxrs.example "1.0.0" {
    shared import javax.javaeeapi "7.0";
    import ceylon.interop.persistence "1.3.1";
    module ceylon.interop.persistence "1.3.1" {
        import maven:org.hibernate.javax.persistence:"hibernate-jpa-2.1-api" "1.0.0.Final"
                => javax.javaeeapi "7.0";
    }
}

Cool!!

@gavinking
Copy link
Contributor

Oh, damn! I just realized that, while this is working great at compiletime, it doesn't have any effect at all at runtime.

The thing is that the existing overrides.xml-based approach doesn't compile-in the overrides; it assumes you're going to provide the same overrides.xml file at runtime that you did at compiletime.

Hrm, I wonder how we can solve this.

WDYT, @FroMage?

@gavinking
Copy link
Contributor

gavinking commented Jul 24, 2017

Hrm, @FroMage, even before hitting the runtime issue, I've run into something else, which you can observe here:

https://github.com/ceylon/ceylon-examples-di/tree/5955

The code passes the typechecker in IntelliJ, but then blows up when attempting to actually compile it with the Java backend. Somehow, the overrides are treated somewhat differently by the command line compiler.

OTOH, in this example:

https://github.com/DiegoCoronel/ceylon-jboss-swarm/tree/5955

everything is peachy, so it's not like they're simply ignored by the command line compiler.

Not sure what to make of this.

@gavinking
Copy link
Contributor

Alright, I think I've got the descriptor / compiletime side of this more-or-less sorted now.

There are currently four things you can do:

  1. set a module version globally:

     module maven:com.sparkjava:"spark-core" "2.3" => "2.5.2";
    
  2. replace a module globally:

     module maven:org.hibernate.javax.persistence:"hibernate-jpa-2.1-api" "1.0.0.Final"
         => import javax.javaeeapi "7.0";
    
  3. add a dependency to a module:

     module maven:org.jboss.weld.se:"weld-se-shaded" "3.0.0.Final" {
         import maven:javax.inject:"javax.inject" "1";
     }
    
  4. replace a dependency of a module:

     module ceylon.interop.persistence "1.3.1" {
         module maven:org.hibernate.javax.persistence:"hibernate-jpa-2.1-api" "1.0.0.Final"
             => import javax.javaeeapi "7.0";
     }
    

I still have not decided how to replace a module (=> null? => void?)

And I will need @FroMage's help on the problem of propagating overrides from compiletime to runtime.

And there is still no "override merging", only the module descriptors of source modules are taken into account.

But anyway, this is actually really good progress.

@xkr47
Copy link
Contributor

xkr47 commented Jul 24, 2017

Nice! Are the version number fields in the "from" part optional in your examples?

module maven:com.sparkjava:"spark-core" => "2.5.2";

@xkr47
Copy link
Contributor

xkr47 commented Jul 24, 2017

What is proposed in this issue is that each module would be able to declare overrides for things it depends on. So overrides would form a hierarchy.

Now since overrides are proposed to be inherited, if you want local overrides only for running e.g. test code, do you propose creating a separate module for testing? Put the test deps that used to be in test-override.xml (optionally along with test code) into its own module that depends on the main module?

@gavinking
Copy link
Contributor

Are the version number fields in the "from" part optional in your examples?

Not in the current implementation.

@bjansen
Copy link
Contributor

bjansen commented Jul 25, 2017

Are the version number fields in the "from" part optional in your examples?

Not in the current implementation.

That could be nice though, if you want to use a policy like "I don't care which version is pulled, I just want to use exactly version x.y".

@FroMage
Copy link
Contributor

FroMage commented Jul 25, 2017

Yeah, most overrides in the wild don't match on version numbers: they blanket replace or remove every version of a given module.

@FroMage
Copy link
Contributor

FroMage commented Jul 25, 2017

replace a dependency of a module

 module ceylon.interop.persistence "1.3.1" {
     module maven:org.hibernate.javax.persistence:"hibernate-jpa-2.1-api" "1.0.0.Final"
         => import javax.javaeeapi "7.0";
 }

I'd go for this syntax to alter dependencies:

 module ceylon.interop.persistence "1.3.1" {
     import maven:org.hibernate.javax.persistence:"hibernate-jpa-2.1-api" "1.0.0.Final"
         => import javax.javaeeapi "7.0";
 }

And this for replacing modules globally:

module maven:org.hibernate.javax.persistence:"hibernate-jpa-2.1-api" "1.0.0.Final"
     => module javax.javaeeapi "7.0";

This way we can stick to module for global matches and import for import matches.

@gavinking
Copy link
Contributor

That could be nice though

Sure, bear in mind that this is still very much a proof of concept, and I'm still just trying to get something that basically "works", before trying to get too deep into the details. I definitely have this feature (all-version matching) in mind.

@gavinking
Copy link
Contributor

I'd go for this syntax to alter dependencies

Interesting. I was coming at it from a different angle:

  • module matches something already-imported somewhere else, and
  • import adds a new import to something.

Part of the justification for the different keyword was that module would eventually have a slightly different syntax to import:

  • the version number will be optional in module, but required in import
  • module can be followed by =>, but import can't be.

OTOH, I can also see how your syntax is also reasonable:

  • module represents a global match, but
  • import refers to a single module dependency.

Both options are reasonable.

@lucono
Copy link

lucono commented Jul 26, 2017

The use of matching keywords on both sides of the fat arrow in @FroMage's proposed syntax seems to me to make it more intuitive than the other alternative.

@gavinking
Copy link
Contributor

The thing is that the existing overrides.xml-based approach doesn't compile-in the overrides; it assumes you're going to provide the same overrides.xml file at runtime that you did at compiletime.

Hrm, I wonder how we can solve this.

The options here are:

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

No branches or pull requests