qooxdoo / qooxdoo-compiler

Compiler for Qooxdoo, 100% javascript
MIT License
36 stars 23 forks source link

Compiler should check qooxdoo-range and warn/abort if not matched #235

Closed cboulanger closed 5 years ago

cboulanger commented 5 years ago

The compiler should check the qooxdoo-range setting in Manifest.json (with a fallback to qooxdoo-versions for v6) and either 1) output a warning if the qooxdoo version used does not match this range or 2) abort compilation unless forced with -f/--force

I vote for option 2)

hkollmann commented 5 years ago

From gitter discussion: What's about adding a about a block neededVersion: { "compiler": "0.2.16", "libraries": { "apiviewer": "0.1.7", "qx": "6.0.0-alpha-20181116-263f2cc9" } to compile.json? Each line should be semver compared to lib and compiler and raise an error if version did not match?

cboulanger commented 5 years ago

Shouldn't it be in Manifest.json? It's a description of actual dependencies which should be enforced, not a compile-time setting.

hkollmann commented 5 years ago

But your description describes it as compile-time setting?

cboulanger commented 5 years ago

Yes, but a structural one that the user cannot/should not change, unlike compile.json which allows ad-hoc user modification. What do you think @johnspackman?

hkollmann commented 5 years ago

ok, i think this are 2 problems:

  1. Compiler needs a checked qooxdoo-sdk version
  2. Application needs checked versions of qooxdo-sdk and contribs.
cboulanger commented 5 years ago

We should have only one SDK, which should be pulled in by the compiler. So the compiler is a prerequisite, not a dependency of the application. The compiler then checks all the dependencies of the application, including the SDK version. It could implicitly do a qx install if it encounters contrib libraries which haven't been loaded yet - that would be useful.

A different question, which should be out of the scope of this issue, is dependency management across contribs, which for the moment should be handled manually. Once we solved the present question, we can then think about recursive dependency management. The point being that the dependencies need to be in the Manifest.json, since libraries might not have a compile.json if they do not contain an application.

johnspackman commented 5 years ago

Everything is a library so IMHO the minimum version of dependencies should be in the libraries Manifest.json ... the catch is that Manifest.json does not specify dependencies currently, because these are specified in compile.json and contrib.json.

@cboulanger IMHO you're right that this would normally go into Manifest.json if we were doing automatic dependency management, the problem is that we only do this in compile.json at the moment.

I'm not against automatic dependency management, but I am a little cautious to avoid the "dependency hell" and the huge amount of code and effort that went into solving it for npm. Perhaps if we declared that the list of dependent libraries/contribs is flattened into a single list before compilation, and that any conflicting versions must be resolved manually that would be ok? But maybe that's how npm started out...

An elegant solution could be to have Manifest.json declare what contribs and versions are required by the contrib (this can optionally include the qooxdoo-sdk with a version, otherwise it would be assumed by the compiler).

The contrib/library's compile.json and contrib.json would be completely ignored when that contrib is being added to some other application/compilation.

The application being compiled would specify its own requirements in its own Manifest.

And :+1: to qx install installing everything recursively

cboulanger commented 5 years ago
cboulanger commented 5 years ago

If we add contrib dependencies to Manifest.json, contrib.json becomes the equivalent of package-lock.json in npm land.

oetiker commented 5 years ago

I think asking libraries to declare their dependencies is a good thing and if the compiler makes sure that no version incompatibilities arise this would be good

ideally, the compiler would even figure out a compatible set of library versions within the declared settings and get these included

cboulanger commented 5 years ago

@oetiker Yes, that would be good. I suggest, however, that we implement a minimal solution first and then wait for more contribs to appear, then implement a more complex solution for real-world use cases. This will both save some work for now (and speed up implementation) and make sure that the solution fits the problem :-)

cboulanger commented 5 years ago

Maybe the qooxdoo-range key in Manifest.json should be deprecated in favor of @hkollmann's suggestion about a whole section on dependencies - but as I said in the Manifest, not in compile.json.

cboulanger commented 5 years ago

Important in this context: the version of the qooxdoo library is not updated when we make a release on master - it stays what it is in version.txt and Manifest.json unless manually updated. That means that we cannot currently enforce a specific alpha.xxx version. That the NPM compiler package states a specific qooxdoo version as one of its dependencies is strictly an NPM issue and has absolutely no bearing on what the compiler actually enforces or is able to enforce. This will change, of course, once we have "real" version numbers such as 6.0.0, 6.0.1, 6.1.0 etc. - or if we changed the build process to update version.txt and Manifest.json on GitHub.

hkollmann commented 5 years ago

to discuss:

append a section requiered to Manifest.json:

{
  "info": {
    "name": "widgetbrowser",
    "summary": "Widget browser",
    "description": "This app demonstrates many widgets of qooxdoo.",
    "homepage": "http://demo.qooxdoo.org/current/widgetbrowser",
    "license": "MIT",
    "authors": [
      {
        "name": "Tristan Koch (tristankoch)",
        "email": "tristan DOT koch AT 1und1 DOT de"
      }
    ],
    "version": "0.1.3",
    "qooxdoo-range": "^6.0.0-alpha"
  },
  "provides": {
    "namespace": "qxl.widgetbrowser",
    "encoding": "utf-8",
    "class": "source/class",
    "resource": "source/resource",
    "translation": "source/translation",
    "type": "application"
  },
  "required": {
      "qooxdoo-sdk": "^6.0.0-alpha",
       "qooxdoo/qxl.versionlabel": "10.1.1",
       "qooxdoo/qxl.formdemo": "0.1.1"
   }
}

Then extent install.js:

  1. to recursive write the requiered contribs from the just installed contrib into contrib.json.
    1. add the new added contrib to Manifest.json. This should be suppressed with command line parameter.
    2. during writing the contribs to contrib.json check for incompatible versions and write out a list.

What do you think?

cboulanger commented 5 years ago

@hkollmann Interesting! Let's see what you come up with. I do think, however, that the compiler itself should enforce dependency checks (which it doesn't ATM) - thoughts, @johnspackman ?

cboulanger commented 5 years ago

@hkollmann maybe use "requires"instead of "required" analogously to "provides"?

cboulanger commented 5 years ago

We have to think about how this relates to the "qooxdoo-range" key. Are we checking library versions, npm package versions, or contrib versions? In the current design, this is somewhat unclear. I personally think that no NPM-related information should be in Manifest.json

hkollmann commented 5 years ago

solved with https://github.com/qooxdoo/qooxdoo-compiler/pull/273