Fix some package detections #100
Conversation
|
Hmm. The linked package test was passing locally (also took me the longest to get working), but that was on Windows. I'll try to debug tomorrow on Linux unless someone figures out what I'm doing wrong before that ;). |
|
|
||
| packagePaths = @getPackagePathsByPackageName() | ||
| for packageName, packagePath of packagePaths | ||
| if packagePath.indexOf('.atom/dev/packages') > -1 or packagePath.indexOf('.atom/packages') > -1 | ||
| if packagePath.indexOf(path.join('.atom', 'dev', 'packages')) > -1 or packagePath.indexOf(path.join('.atom', 'packages')) > -1 |
There was a problem hiding this comment.
oh man. Cant believe this has been in there so long.
There was a problem hiding this comment.
I think this still might be incorrect...@kevinsawicki if someone sets their ATOM_HOME to something like /Users/user/ATOM, then will this check fail? If so, how would I fix that? By reading process.env.ATOM_HOME and then regexing it to capture the last folder path?
There was a problem hiding this comment.
The goal of this line is to check if the package is bundled or not right?
There is atom.packages.isBundledPackage and atom.packages.isBundledPackagePath that might be useful to use instead.
There was a problem hiding this comment.
I think it's actually to check if a package is symlinked or not, and then to get its true path since that's where the error will have been thrown from.
Eg if I have notifications linked to .atom\packages\notifications from C:\Users\user\Documents\Github\notifications and notifications throws an error, then this piece of code will correctly match the two.
There was a problem hiding this comment.
Oh yeah, I'd say maybe just call fs.realpath in all cases then if we want to ensure the stack paths match the package paths.
|
👏 @50Wliu Going to cc @kevinsawicki for more 👀 since he worked on some improvement to this package and error detection, if I remember correctly. |
| @@ -190,6 +191,9 @@ class NotificationIssue | |||
| return packName unless /^\.\./.test(relativePath) | |||
| @getPackageNameFromFilePath(filePath) | |||
|
|
|||
| packageName = /Failed to (load|activate) (the|a) (.*) package/.exec(message)?[3] | |||
There was a problem hiding this comment.
I wonder if we should pass in the packageName as metadata to the notification instead of having to regex the message.
There was a problem hiding this comment.
Yeah, that would probably be better. Any pointers on how to do that?
There was a problem hiding this comment.
Yeah, I wonder if putting it in this payload would be sufficient, https://github.com/atom/atom/blob/666bb344647d1f96df93c875e404e32bd6c1d419/src/package.coffee#L637
There was a problem hiding this comment.
And here: https://github.com/atom/atom/blob/666bb344647d1f96df93c875e404e32bd6c1d419/src/package-manager.coffee#L470 (and maybe in other places)
For reference, grim supports a packageName in the metadata: https://github.com/atom/grim/blob/82c45ef794ce93e4455c29365441c62bf94b90a8/spec/grim-spec.coffee#L108
|
Unfortunately I can't reproduce the failing spec, even on Ubuntu 😞. |
|
Wow ok so I totally forgot that package specs run on Mac and not Linux. Can anyone repro the failing spec on Mac then? |
|
Also, should we switch to actually building and then loading a package (similar to how spec/package-manager-spec.coffee does it) instead of manually creating the notification ourselves? |
| @@ -190,6 +190,8 @@ class NotificationIssue | |||
| return packName unless /^\.\./.test(relativePath) | |||
| @getPackageNameFromFilePath(filePath) | |||
|
|
|||
| return options.packageName if options.packageName? | |||
There was a problem hiding this comment.
Seems like this could be returned at the top of this method? Like line 178?
Node stack traces seem to always contain real-paths, even if the file was required via a symlink path
|
🚢 |
Whoops, missed that... |
|
Thanks, @50Wliu! |
This fixes the following scenarios:
apm linked on a Windows machine throws an errorAll of those would previously fail to find the correct package to report the issue to and so they were all dumped to atom/atom.
Refs #66
Fixes #98