noseglid / atom-build

:hammer: Build your project directly from the Atom editor
https://atom.io/packages/build
MIT License
248 stars 97 forks source link

Support non-file specific errors #463

Closed ProPuke closed 7 years ago

ProPuke commented 7 years ago

Currently the filePath is mandatory when handling build errors. It would be nice if it could be omitted so that general build errors could be displayed, too.

Some compilers or build systems may throw errors which are not specific to particular file locations, but should nonetheless be reported to users. While these can be seen if the build panel is open, if it is hidden these can go undetected.

It is possible to pass errors to Linter without a filePath (in which case it adds to the error total at the bottom, but does not display the error message anywhere). This could be used along with a standard atom error notification to show such errors.

Alternatively, Build could allow the provider to provide an onErrors callback for handling all reported errors including ones without filePaths, in which case it could perform special handling if it needed to. Or even just a general callback like onData so that it could manually handle any special output produced by the build command.

Cheers

Cxarli commented 7 years ago

I think that you shouldn't rely on build to give you all information before you commit. Always make sure that it runs good in your terminal too, and maybe add CI (like Travis) to your project if it matters (production environment).

I myself would be frustrated if my error counter would give 2 and I could only find 1 error in my file, and no other error elsewhere.

I also think that it would be really hard for a package like build to distinguish between compiler-error and compile-error. (Especially since you don't provide an example)

However, I think there is / should be a function in the build API which allows a 3rd party to handle all errors? In that case, you could make an extension which handles these compiler-errors (which I myself haven't ever found yet)

ProPuke commented 7 years ago

An example might be compiling C/C++ Sometimes you'll get compiler errors:

file.cpp:1:1: error: foo is foo

And sometimes you'll get linker errors:

Undefined reference to "foo"

They're both build errors, but atm there's no way of handling the latter with Build, even from custom Build packages. If you want to the only route currently is to drop Build and write your own package from scratch.

Distinguishing would be no problem. You'd write matching regex statements as you currently do.

For now I'd suggest just using standard atom error notifications if there isn't a file location, so it isn't just a counter with no informative message. (although if there were too many I realise this could flood the screen)

If not, just the ability to hook these errors or the build output and handle it yourself would be fine; Then you could handle it appropriately from your own package.

Cxarli commented 7 years ago

In reply to your C/C++ linking error; as far as I know, g++ (and possibly gcc) gives the file with the error even on linking errors.

// test.cpp 
#include "linkerror.h"

int main() {
  Link link;
  link.error();
}
// linkerror.h 
class Link {
public:
 void error();
};
$ g++ -o test test.cpp
/tmp/ccappBOa.o: In function `main':
test.cpp:(.text+0x1f): undefined reference to `Link::error()'
collect2: error: ld returned 1 exit status

(test.cpp is stated here, even the function in case that's needed)

But I get what you're trying to say.

The regexes would indeed be possible, just as an atom notification.


Now to the implementation, what would be a way to collect these errors to generate regexes?

ProPuke commented 7 years ago

Linker errors do not always include file locations. This is not always possible.

To be honest I don't have any suggestive matches for other build targets. I'm currently using Build for a custom language, and this is a feature I find myself needing. Although I do think it would be useful for other build systems too. If it's the first barrier I've hit when trying to integrate with Build, I'd imagine it likely to be for others as well.

alygin commented 7 years ago

Atom-Build uses Linter to visualise build messages. filePath is optional in Linter for now, but it's going to change in the next version. So, making it optional in Atom-Build will complicate switching to Linter 2.

We had a similar problem in atom-build-cargo and decided to use Notifications for non-compiler messages for which we cannot be sure we always have a file link. Screen cluttering is prevented by enforcing the limit on the count of notifications that can be displayed at once. We display ten notifications and one more with a text like "27 more errors are hidden". Maybe, it's the way to go in this case too?

ProPuke commented 7 years ago

Yeah, I ended up skipping Linter for errors without filepaths in my PR, as it wasn't optimal as @C-Bouthoorn pointed out (the linter counter had no associative link with the errors, which was weird).

I ended up collating them and displaying them all in a single error/warning notification, so worst case you have a single big notification that scrolls.

I also added stdout and stderr arguments to postBuild, so you can access build output yourself if you want, and handle things however's appropriate.

noseglid commented 7 years ago

Hmm, displaying general errors may or may not be useful. I'm afraid it might cause some confusion though. Since it will be mandatory in the next version of Linter (as @alygin pointed out) I don't want to remove this requirement.