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

Confirm that exit status is non-zero on compile error #440

Open
derrell opened this issue Jun 7, 2019 · 11 comments

Comments

@derrell
Copy link
Member

commented Jun 7, 2019

For unattended operation, e.g., to compile the app from gitlab-ci, it is important that if the compiler detects an error, it exits with a non-zero status. According to @hkollmann it may already do this properly, but @johnspackman requested an issue to confirm that it does.

@derrell derrell added the blocking label Jun 7, 2019

@hkollmann

This comment has been minimized.

Copy link
Member

commented Jun 7, 2019

Any idea how to test this?

@derrell

This comment has been minimized.

Copy link
Member Author

commented Jun 7, 2019

There are at least two categories of errors that should result in non-zero exit status:

  • a compile error, e.g., invalid JavaScript
  • valid JS, but includes a reference to a class that can not be resolved, e.g., a library is not available or a name is mistyped in an @require, e.g., a "link" or dependency error

I suspect there are additional, common categories of errors but identifying them probably requires better understand than I possess regarding the internals of this compiler.

@cboulanger

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2019

Shouldn't all errors result in non-zero exit status?

@derrell

This comment has been minimized.

Copy link
Member Author

commented Jun 7, 2019

Yes.

I took the question to be how to write some test cases that should cause errors to confirm that non-zero exit codes result.

@cboulanger cboulanger added this to the 1.0.0-beta milestone Jul 9, 2019

@cboulanger

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2019

@derrell @johnspackman What is the status of this issue?

@johnspackman

This comment has been minimized.

Copy link
Member

commented Aug 20, 2019

i don't think it's fixed yet

@johnspackman johnspackman self-assigned this Aug 23, 2019

@oetiker

This comment has been minimized.

Copy link
Member

commented Sep 5, 2019

Just looked through the code and it is not immediately obvious how to tackle this.

--- a/source/class/qx/tool/compiler/makers/AppMaker.js
+++ b/source/class/qx/tool/compiler/makers/AppMaker.js
@@ -164,7 +164,7 @@ qx.Class.define("qx.tool.compiler.makers.AppMaker", {
             application.calcDependencies();
             if (application.getFatalCompileErrors()) {
               qx.tool.compiler.Console.print("qx.tool.compiler.maker.appFatalError", application.getName());
-              return undefined;
+              throw new Error("FatalError");
             }
 
             this.fireDataEvent("writingApplication", application);

tried the above, but it seems a whee bit drastic and also breaks --watch

@johnspackman

This comment has been minimized.

Copy link
Member

commented Sep 5, 2019

where is that throw new Error("FatalError") ? It's not in the current master https://github.com/qooxdoo/qooxdoo-compiler/blob/master/source/class/qx/tool/compiler/makers/AppMaker.js#L165-L167

@oetiker

This comment has been minimized.

Copy link
Member

commented Sep 5, 2019

where is that throw new Error("FatalError") ? It's not in the current master https://github.com/qooxdoo/qooxdoo-compiler/blob/master/source/class/qx/tool/compiler/makers/AppMaker.js#L165-L167

That is a little experiment I did :) to see if there was a quick fix. It is just in my local checkout.

@johnspackman

This comment has been minimized.

Copy link
Member

commented Sep 5, 2019

aha! :) fixing it now

johnspackman added a commit to johnspackman/qooxdoo-compiler that referenced this issue Sep 5, 2019
@johnspackman

This comment has been minimized.

Copy link
Member

commented Sep 5, 2019

johnspackman added a commit to johnspackman/qooxdoo-compiler that referenced this issue Sep 5, 2019
Merge branch 'issue440'
* issue440:
  fixes qooxdoo#440
  for windows we need upath here (qooxdoo#563)
  syntax fix
  add more node versions to test with
  add shell: true to support windows
  npm link seems to be needed
  fix qx path
  make windows compatible
  bring some color to output
  fixes regression for multiple applications; (qooxdoo#561)
  use rimraf async
  more echo output
  fix linefeed
  fix lint
  fixes for windows
  use upath instead of path
  use npx instead of qx global
  remove unneeded npm  link
  add windows to travis test platforms

# Conflicts:
#	test/test.linux.sh
#	test/test.win32.cmd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.