Fix upgrading #2176
Fix upgrading #2176
Conversation
kenny-evitt
force-pushed
the
fix-upgrading
branch
2 times, most recently
from
8439115
to
3c31365
Compare
|
@cldwalker @rundis I've basically got this working. It seems to work entirely on Windows but not on Mac (and I haven't even tested at all on Linux yet). On my Mac, the code to delete the temp files (the tmp sub-directory and the actual downloaded archive file) is failing (and silently). I'm debugging it now. I'm also going to separate the current WIP commit into at least two separate commits – the first with just the new Node modules I added. As-is, there are too many files to view the actual code changes in GitHub! Once I've got the code working for me on all three platforms, then I'll start cleaning up my changes. |
'tar-fs' supports passing an 'fs' module explicitly, which is needed to work around Electron patching 'fs' to make '.asar' files behave transparently as native directories (which breaks a lot of code). 'node-stream-zip' is needed to unzip '.zip' release files for upgrading Windows computers.
kenny-evitt
force-pushed
the
fix-upgrading
branch
3 times, most recently
from
e6cde6d
to
a348fbe
Compare
kenny-evitt
force-pushed
the
fix-upgrading
branch
from
a348fbe
to
7867d36
Compare
|
@cldwalker @rundis I've got this working on all platforms. There's definitely room to cleanup the code. But I'd like to get some feedback from both of you – or at least one of you – before I start working on that. To test, you can checkout the branch 0.8.0-fix-upgrading-test in my fork and build from source. I cherry-picked my two commits in this PR on top of the code in master as-of the 0.8.0 tag. |
|
Here's the files-changed diffs for just the "WIP" commit (as the other commit only adds files for two Node modules I added). |
| (-> p | ||
| (string/split " ") | ||
| (second))))) | ||
| ;; (defn proxy? [] |
There was a problem hiding this comment.
I commented this out because it calls tar-path, but that now requires a callback, and I couldn't find any callers of this function anyways.
|
The shitty change – that I can't figure out how to avoid without even shittier (or much harder) changes – is that I had to explicitly handle .asar files in our (previously) minimally modified shell.js Node module fork. |
|
@cldwalker @rundis This could definitely use a thorough review, ideally from both of you. |
|
@kenny-evitt: I build LT from the 0.8.0 tag cherry picking the commit WIP and although it correctly builds, I get an error on the console when starting LT. Here is what I've got: It seems that due to that error LT is completely useless. I can't open workspace nor add folder manually. The search bar barely works and things like "About LT" or the "Plugin manager" don't open either :/ |
|
@carocad Thanks! Would you cherry-pick both commits, build, and then try testing the upgrade again please? The error refers to a Node.js package added in the commit prior to the "WIP" commit. |
|
@kenny-evitt I did as you said. The build process succeds without any problem, LT starts without a problem. But it all ends there :( A final note: Something not so funny now happens on my normal LT. Everytime that I open my normal LT it asks me to install paredit and rainbow, which I purposely deleted before ! Hope it helps EDIT2: I was mistaken. The update does succeed !! :D I thought it didn't because there was just no notifo saying anything. After some minutes I got the "Lighttable is updated to its latest and greatest" pop-up. |
|
@carocad Was the CHANGELOG updated? It should include a section for 0.8.1 at the top. And you should have seen a "Light Table has been updated!" popup when the upgrade finished that suggested you restart LT too. I'll look into this soonish. |
|
@kenny-evitt yes to all your questions. It works as expected |
|
@carocad Oh! Cool, thanks! |
|
Hey @kenny-evitt I tested your changes and all looks good to me! Here's what I did:
#2193 - Can’t reproduce. OSX 10.11.5, MBPro Late 2011 |
|
@martinchooooooo Thanks! |
|
I was able to test this out as well: Commands to start off:
Excellent work @kenny-evitt! Only comment I can think of is that we should clean up the commented out code and take care of the lingering TODOs introduced in the PR. |
|
@rundis This is a significant enough change that this needs your approval before it can be merged. We should also decide how to address or resolve (or not) the various points I mentioned in this comment on the issue – #2138 – to which this PR corresponds:
I hazily remember @cldwalker bringing up something about the Node.js packages. I think the first commit in the PR not only adds two packages I used explicitly, but it also included other packages added when I ran I also need to do the following before this merged:
|
|
@sbauer322 Thanks for testing this! |
| v)] | ||
| (fetch/xhr release-info-url | ||
| {} | ||
| (fn [data] |
There was a problem hiding this comment.
That's a whole lotta' lets !
How about something like :
(fn [data]
(->> (-> (js/JSON.parse data)
(js->clj :keywordize-keys true))
:assets
(filter #(< -1 (.indexOf (:name %) (name platform/platform))))
first
:browser_download_url
cb))
There was a problem hiding this comment.
I'm not against your suggestion, and it's definitely much more idiomatic Clojure, but it's much harder to read for me. You also subtly changed the behavior; did you deliberately remove using the version (v)? Doing so is probably (?) okay, as I don't think we even have (or want to provide) a way to upgrade to any version that's not the latest, my version was (probably) intended to explicitly use the version specified.
There was a problem hiding this comment.
I do agree this suggestion is harder to read, but feels cleaner once you stare at it for a few minutes. In particular, using the results of -> as the first argument to ->> was initially disorientating.
Maybe use a let binding outside of the ->> instead of:
(-> (js/JSON.parse data)
(js->clj :keywordize-keys true))
to improve readability?
Not to self-promote poorly written blog posts (of which I am a fine provider of), but I wrote a rather brief guide about arrow macros once.
There was a problem hiding this comment.
@kenny-evitt The v is used for the rest / ajax call, I just didn't bother also using it in the callback traversing the result... as it would be very surprised if we had multiple versioned release artefacts for one release tag.
Both: Yeah I suppose @sbauer322 suggestion is a case where let would help readability.
I'll admit that I use threading quite extensively. Once you get used to them, they read actually quite a lot easier than the normal way to read Clojore/Lisp (inside->out). I think of them like a pipeline which you can just read sequentially.
Anyways I do think that using a let just to set up the following let that sets up the next let etc is a very imperative approach (and all those names doesn't really add that much, because they are just a means to an end - getting that download url. Come to think of it it could be extracted to a separate function) The multiple let approach just doesn't feel particularly functional to me. I guess that was my main point :-)
| ;; (when (str-contains? p "PROXY") | ||
| ;; (-> p | ||
| ;; (string/split " ") | ||
| ;; (second))))) |
There was a problem hiding this comment.
no biggie, but you don't actually need the parenthesis when threading and the function in question just takes one arg.
There was a problem hiding this comment.
But I see this is not your doing in the first place...
There was a problem hiding this comment.
Note also that I'm commenting out this function entirely. My original comment about these changes:
I commented this out because it calls
tar-path, but that now requires a callback, and I couldn't find any callers of this function anyways.
|
@kenny-evitt Maybe this would help with the ASAR stuff ? electron/electron#3641 It would definitely be good if we were also able to make upgrades to Electron itself. Otherwise that will become another source of bitrot too. |
| @@ -389,8 +389,8 @@ function _rm(options, files) { | |||
| // If here, path exists | |||
|
|
|||
| var stats = fs.statSync(file); | |||
| // Remove simple file | |||
| if (stats.isFile()) { | |||
| // Remove simple file (or Electron package files which 'fs' has been patched by Electron to treat as directories) | |||
There was a problem hiding this comment.
Is that still true with electron/electron#3641 ?
There was a problem hiding this comment.
It seems like it's not true with that PR merged. What's weird is that PR was merged on the same date as we last bumped Electron in LT. But we bumped to 0.34.5 and that PR seems to be part of 0.35.3. @sbauer322 might test upgrading to a newer version.
I'll try to do so myself for the changes in this PR too. Given that everyone's going to need to either download the next version or build it themselves from source, upgrading Electron now has no marginal impact. Not having to handle the ASAR workaround should simplify this greatly. Maybe we could even get rid of some other Node.js dependencies.
Well I do care about dependencies. The fewer the better. I suppose I don't have to mention the left-pad saga with NPM ? There is an npm package for checking whether something is an array or object. That's f*ng oneliners. All these deps are increasing our maintenance burden, bloating the distribution and dumbs us down because we don't care enough about what these things do. It's just a matter of adding a thing to our package.json and behold the internet being pulled down. I'm going O.T. I'm not saying we should write our own un-taring, but do we need that new dep (and all it's transitives). I'm not sure what's wrong with the one we had. Do we need to keep both ? |
|
@rundis Thanks for the comments! I'll need some time to go thru them all; at least more thoroughly. What I meant about the Node.js packages, specifically, was something to do with them being vendored strangely in the repo. I only added two packages and, yes, their dependencies would be expected to be added to the repo too, but there were also other packages added (I think) that are dependencies of other existing packages. I can't find the original conversation, wherever it was, so I don't remember exactly. I didn't think the packages had that many dependencies but I just checked out the graph for tar-fs: ... and – sigh – you're right that that looks crazy. |
The goal is to download the appropriate release file for each platform. Currently, the source tarball is being downloaded but that doesn't contain any of the compiled ClojureScript code, so nothing is really upgraded.
This is currently definitely a work-in-progress.