atom-community / atom

:atom: Community build of the hackable text editor
https://atom-community.github.io/
MIT License
744 stars 30 forks source link

Executable Versions + Script Runner #111

Open jeff-hykin opened 4 years ago

jeff-hykin commented 4 years ago

AFAIK, there isn't a standard place for mentioning versions of binaries. For example, what version of powershell is being used for the CI scripts, and versions of python are needed for node-gyp for building native modules.

Ideally there would be a standard way to list versions of these binaries (outside of atom) but I'm unaware of any standard. So, in substitution of that, could the versions be added to the package.json as a "bin" or "binaries" or "executables" object?

I think this would be helpful since it's already a struggle trying to build atom.

DeeDeeG commented 4 years ago

You mean to specify some versions in the project somewhere, so it will warn or prevent you proceeding if you've got a build setup that won't work?

An interesting thought, although I think we should focus on testing a set of supported build setups and documenting which are supported.

I just did a very large amount of research and testing as to what works for building Atom today that you may be interested in: https://github.com/atom/flight-manual.atom.io/pull/630#issuecomment-678666913

DeeDeeG commented 4 years ago

We already have a good way of detecting Node, npm and Python and stopping you from bootstrapping if you've got the wrong versions.

See: https://github.com/atom/atom/blob/2f550cf0d38693598bbba1d2abf34e176a9e8ece/script/lib/verify-machine-requirements.js

_(Edit: This is a bit out of date for Node version, though, as of https://github.com/atom/atom/pull/20879. (electron-mksnapshot 9.0.2 and electron-chromdriver 9.0.0, in script/package.json, have an indirect dependency that requires Node 10.12+). And it hasn't been updated to deny Node 12.17 or newer, which was released at the end of May this year with a breaking change for babel.)_

It's surprisingly difficult to properly detect a working Visual Studio install. And node-gyp will tell you if you've got no working Visual Studio installs. I'd like to leave detecting Visual Studio to node-gyp.

(Long aside: We have a fix for apm that makes apm get out of the way so node-gyp can properly detect a working Visual Studio install, but this only really helps in a niche situation (broken Visual Studio 2015 or older, installed alongside working Visual Studio 2017 or newer). It's already mostly working in upstream apm.)

I would be shocked and surprised if powershell version or cmd.exe version had any impact on a successful build. Okay, I've seen a lot of things, so it wouldn't be that surprising. It's my sense that that isn't the case though, absent specific testing that shows otherwise.

aminya commented 4 years ago

I think this would be helpful since it's already a struggle trying to build atom.

@jeff-hykin

10 will solve this issue partially. You will not need anything to start hacking the core using the pre-bootstrapped repository.

To build the Atom, I just install windows-build-tools, and I use the node that is provided by apm. I think we can add a script to find that node version automatically. After the unification that we made, this step is necessary.

One other way is to use a docker file. We have a docker file but it is out of date.

See: https://github.com/atom/atom/blob/2f550cf0d38693598bbba1d2abf34e176a9e8ece/script/lib/verify-machine-requirements.js

I need to update this script. This is missing a couple of things.

jeff-hykin commented 4 years ago

Docker would be nice. I'm on Mac but I triple boot and I'd like to generate a redistributable app for all three OS's.

so it will warn or prevent you proceeding if you've got a build setup that won't work

Idk if that can be expected to be reliable. I've tried node 14, 12, 10, 11, python 2 and 3.6, 3.7, 3.8, disabling the GitHub extension, upgrading downgrading npm packages, deleting node_modules and the other usual suspects, resulting in different build failures. I've got the full install of xcode and the other prerequisites on the flight manual.

Just now (after changing nothing inside the folder for a week), the build script completed for the first time. But as luck would have it, the Dev Atom immediately fails on opening with a Cannot find module '../build/Release/nslog.node'.

A package-lock style of guaranteed-to-work versions of node, python, etc would just be nice so that I can rule those out when trying to debug why the build is failing.

To build the Atom, I just install windows-build-tools, and I use the node that is provided by apm.

@aminya How do you use the node that is provided by apm? (edit the build script or PATH?)

jeff-hykin commented 4 years ago

(Thanks for the help btw)

aminya commented 4 years ago

To build the Atom, I just install windows-build-tools, and I use the node that is provided by apm.

@aminya How do you use the node that is provided by apm? (edit the build script or PATH?)

Yes, you can edit your path so the Node provided by apm is the first.

Yes, I said, I need to change the scripts so that Node version is automatically found! The script that we have which checks the versions is out of date and does not reflect the actual requirements!


For now, you can download and use the following 😄. Use the absolute path to this to run the scripts. Like:

C:/node.exe ./script/bootstrap

node.zip

To make this Node version yourself, run npm install in the atom/apm path. A node.exe will appear in the bin folder (node_modules/atom-package-manager/bin)

DeeDeeG commented 4 years ago

How old is your mac? Are we talking recent like Mojave or Catalina? Older than that? (Should still work, but it would be nice to know.)

Also, having cd'ed into the Atom repo, can you do git remote update and git status? Are you up to date with the latest master? Are you building upstream or this fork? Are you on a custom branch?

Edit to add: It's a good habit to do script/clean then script/bootstrap and script/build --no-bootstrap if you want a clean build environment. If you haven't found a reliable-to-build commit of the repo yet, try this on upstream's master branch, or maybe even better, a release tag like v1.50.0.

Lastly: How much RAM and free disk space do you have? ~4GB is enough RAM, but could cause issues if you also have a lot of other things open. Especially web browsers, even moreso with lots of tabs open. If you're almost 100% out of disk space, things could get dicey during install, files not ending up where they're supposed to, etc.

I am on Linux and Windows, and builds there are fairly reliable on my real machines on command-line. (As opposed to CI, which is its own separate thing)

How do you use the node that is provided by apm? (edit the build script or PATH?)

Technically you could copy that somewhere and add it to your PATH... But, uh... on Windows I just download the x64 .zip from here: https://nodejs.org/dist/ extract to somewhere and add it to my PATH. And on Linux I use n. n works on macOS as well. Latest Node v10.x is a safe bet for now. Otherwise Node v12.16.x. Node 12.4.0 is the version you should use.

I've got the full install of xcode and the other prerequisites on the flight manual.

For macOS, you should just need these on your PATH:

Nothing else should be required whatsoever in terms of system requirements (on macOS). But willing to troubleshoot with you.

jeff-hykin commented 4 years ago

~Latest Node v10.x is a safe bet for now. Otherwise Node v12.16.x.~ Node 12.4.0 is the version you should use

Haha, yeah thats why I think they need to be locked to a specific version.

I'm on node 12.0.0 which is probably why the build worked this time, I was previously on 12.16.x

At the moment I'm testing on a Hackintosh, 48Gb ram, running Sierra, but I've got mac books with Mojave and High Sierra I can build on as well. I run a lot of projects with a lot of different versions so switching isn't too bad, I use asdf, nvm, and nix for managing versions.

Note: npm install atom-package-manager quickly fails with node v12.0.0, on node v12.4.0 it fails on node-gyp (even after clearing node_modules deleted both times), but I decided to try on node 12.18.3 and sure enough npm install atom-package-manager works on that. Now I'll see if I can build atom with the 12.4.0 node that I had to get using 12.18.3 haha.

Screen Shot 2020-08-26 at 7 34 04 PM

Note the first Node is 12.4.0, then the second one is Node 10.20.1 x64 which is weird

Okay, I've seen a lot of things, so it wouldn't be that surprising. It's my sense that that isn't the case though

I don't think this is a good policy. It takes 1 minute to add a powershell version to a package.json or somewhere else. The "Node.js 6.x or later" thats still on the Flight manual was probably also a "It's my sense that that isn't the case" recommendation, which has cost me and others days of debugging. I'd bet most people trying just give up and don't contribute. I don't mean to sound attacking; you guys are helping me out and I see you contributing everywhere. But the CI tests are already spotty right? pinning down the versions seems like the best action to start making builds more reliable. It takes 5 min to record all the working versions you have, it can even be automated; but, on my end, checking all the possible combinations of versions is nearly impossible. Why would you not just preemptively record the versions?

jeff-hykin commented 4 years ago

Hey @aminya your trick worked, Atom Dev actually opened without crashing 🎊 🎉 Thanks 👍

aminya commented 4 years ago

Thank you Jeff. I will update the scripts such that the versions are pinned. Currently, the only version of Node that I support is 12.4.0. It should build everything from Atom to apm itself.

I don't mean to sound attacking; you guys are helping me out and I see you contributing everywhere. But the CI tests are already spotty right? pinning down the versions seems like the best action to start making builds more reliable. It takes 5 min to record all the working versions you have, it can even be automated; but, on my end, checking all the possible combinations of versions is nearly impossible. Why would you not just preemptively record the versions?

That is totally true. I have had this issue myself. That's why I created #10. Using that PR you can edit the JavaScript code right away! It is like a prebuilt docker image!

Additionally, I am also trying to pin and unify the versions everywhere. A single pinned version makes sure that everything works. For example, different node versions in Atom and APM created a lot of issues for us.

Currently, this file includes all the versions I recommend: https://github.com/atom-ide-community/atom/blob/master/script/vsts/platforms/templates/preparation.yml

I am thinking of making a preparation.ps1 and preparation.sh that installs all the dependencies (for different OS) similar to our CI.

Using a container is another solution, but not all people have experience with using those. A preparation.ps1 is more accessible than a docker container.

jeff-hykin commented 4 years ago

That's why I created #10. Using that PR you can edit the JavaScript code right away! It is like a prebuilt docker image!

That sounds fantastic, I'll have to look into that further.

Additionally, I am also trying to pin and unify the versions everywhere. A single pinned version makes sure that everything works. For example, different node versions in Atom and APM created a lot of issues for us.

When I propose changes, I'll try to help with that. I noticed just now that oginumura and the tree sitter are using native modules even though there's WASM builds for both so I'll be trying to swap those out to lessen the dependency hell.

aminya commented 4 years ago

When I propose changes, I'll try to help with that. I noticed just now that oginumura and the tree sitter are using native modules even though there's WASM builds for both so I'll be trying to swap those out to lessen the dependency hell.

That's awesome! I am looking forward to your contributions!

Currently, Atom does not tree-sitter by default, and you should enable it in the settings. I was thinking about making them the default. I'll wait until you bring those changes, and we can switch to tree-sitter as the default language mode.

DeeDeeG commented 4 years ago

Hi again, folks. I do not doubt your personal experiences, but if there are specific failures on various Node versions, we should document them, as they may be specific bugs we can solve. The reason most projects do NOT pint to an exact version is that you simply don't have to. The code builds on many Node versions. It is bordering on superstitious to pin to an exact Node version. Most Node code is Node version agnostic to a certain degree forward and back. It is certainly less informative (and less proactively healthy for the code) to pin a Node version, rather an tracking down and solving these bugs. Though admittedly, Atom runs on fixed versions of Node/Electron, so the (direct) impact to end-users is none. It's more of a "looking out for developers' sanity, long-term" thing. My two cents. Glad to see you are having more success now building Atom, Jeff.

DeeDeeG commented 4 years ago

Also please be careful doing npm install with no flags in the apm/ folder. That can actually break the install.

It should be npm install --global-style, just for the apm folder. Otherwise apm can't find its bundled node and things like that.

(This is due to a weird workaround for using apm with npm@5, as apm was first written a few major versions of npm ago before npm learned how to hoist and dedupe repeated dependencies, and somewhat flatten the node_modules dependency tree as extracted to disk. Which changes the place that various things get installed to relative to the atom-package-manager module dir. Some paths are hard-coded inapm and can't handle things moving around. Ideally the apm scripts would be re-written to be smarter about finding the bundled npm and apm launcher scripts, and the bundled node and so on. I haven't thought of the right way to do this, but it had been a goal of mine to make that happen if possible, eventually.)

aminya commented 4 years ago

The reason most projects do NOT pint to an exact version is that you simply don't have to.

if the code is only JavaScript that is not a problem because Node is not breaking most of the time, but native modules are required to be built against the same Node versions that run them! There is no way you can build a native module on a different Node version and run it.

See this document so you get familiar with this: https://www.electronjs.org/docs/tutorial/using-native-node-modules

That is why we shall use the same Node version in both apm and Atom.

The specific projects that use prebuild or node-pre-gyp may run in Electron without rebuilding from the source.

DeeDeeG commented 4 years ago

First of all I have no problem with this:

That is why we shall use the same Node version in both apm and Atom.

If you want to run a system Node version synced with apm, matching the Electron version Atom is using, that's no problem. Doing it in CI is also no problem. In contrast to that, telling people they have to do so is basically misinformation and I will push back on that.


What you say is true. But consider this additional information, which is true as well:

All code is executed by the Node that installs it, (or built for/executed by the Electron that runs Atom) so there is theoretically no problem if multiple different versions are used. There is no particular reason they need to be in sync. (The build scripts in script/node_modules should run the same once they're built, regardless of what Node version they are built for and run on. They should produce the same built Atom package, regardless, or it is an unintended bug.)

(Real-world example: Atom was on Electron 5 (~Node 12.0.0) while apm had bundled Node 10.20. For multiple stable releases of Atom. And you know what? It worked fine. That's because apm's bundled Node is only used to execute apm. apm passes flags to npm and node-gyp that tells it to build code for an Electron target. Not Node.)

Again, it would be really ideal to track down any situation where this is not the case, document it, make it reproducible, and squash the bug.

I continue to recommend using the most stable/reliable system Node possible, and nothing more than a year old. (Newer means more bugfixes from Node's developers, I guess.) Doesn't matter what version exactly. What works for you to build Atom is all that really matters. And for apm's bundled Node, I agree it should match Electron as close as possible, because there were bug reports at the upstream apm repo about the mismatch causing problems. (In theory this is only for major versions, due to no breaking changes within the same Major version during Node's Long Term Support period, so we could perhaps bundle Node 12.16 in apm, but that would need to be tested and I don't want to waste time testing it or bump Node in apm willy-nilly when Node 12.4.0 is working just fine already.)

I know it's easier to think about using one version of Node everywhere, but it's even better to understand our technology stack and not be afraid of the moving parts. And to proactively fix bugs when we find them rather than pinning Node as a workaround. Hacking on Atom doesn't (shouldn't) require an exact version of Node on the system PATH, for convenience's sake. Developers should confidently build Atom from whatever recent-ish Node they have installed. If something breaks on other versions of system Node, it's a bug.

Sorry to keep saying this, but it's true every time I say it.

aminya commented 4 years ago

I wonder how you explain the issue in https://github.com/atom/apm/pull/889#issuecomment-671160524.

DeeDeeG commented 4 years ago

Those ABI versions in the error message (ABI 70 and 73) are reserved strictly for Electron.

https://github.com/nodejs/node/blob/7aeff6b8c8e1b7df2e9c1b7638c1c441af6ff45d/doc/abi_version_registry.json#L17-L20

(Node and Electron no-longer share ABI versions starting roughly January 2019. https://github.com/nodejs/TSC/issues/651)

According to the error message, the code was built for electron 5, but trying to run in Electron 6.

The only logical explanation (barring very severe bugs) is that you accidentally had code (an Atom package) installed for one major version of Electron and tried to run it on a newer Electron around the time of the Electron 6 bump.

We do switch branches a lot working on various things, so this is a totally understandable situation, in my opinion. I don't know the exact details of how they happened, but the error message is pretty clear about what's going on, once you know what place to look up those ABI version numbers.

There is no reasonable way that Node version bumps could cause those errors.

aminya commented 4 years ago

I did not build that Atom. I just downloaded the artifacts from the Electron 2 pull request, and rebuilt the package using Atom's included apm, and it did not work. The error message did not change after tying many times.

The whole point is that Atom's runtime should use the same Node version of Electron for building the native modules.

It is OK to rely on being lucky sometimes, but I don't want to advertise that in the documentation. There is no point in using different Node versions recklessly just because we can.

If we want to remove this issue completely, we should use electron-rebuild. The apm rebuild code is kind of wonky, and I can't trust it.

DeeDeeG commented 4 years ago

I think the apm command you were using on the command-line may have been pointed at an older copy of Atom, thus building against Electron 5. And it built the package in the shared [User]/.atom folder... ultimately mismatched to the Electron 6 you were trying to run it against. That is a totally believable explanation in my opinion. If you have multiple Atom installs side-by-side, this is bound to happen eventually by mistake.


If apm's bundled Node had anything to do with it, the ABI version in the error message would have been expected to be ABI 64 (Node 10) or ABI 72 (Node 12).


We can pin Node in CI if you want, but I happily build Atom on my personal machine with Node 10.x, and anyone else who wants to use a version of Node other than 12.4.0 is welcome to post bugs here if they run into problems, and I'll help troubleshoot them in my spare time.

10.12 through to the latest 10.x should work, as should Node 12.0.0 through 12.16.3. If one of those doesn't work and somebody wants help to build Atom, I'll help them do so. That's my personal take.

What Node versions should work isn't a matter of opinion, it's the facts of our build stack. I really don't want to argue just for argument's sake, so at some point I'll stop, but my opinion hasn't changed on this. (If you're running into errors, you may be running a different build technology stack than you thought you were running. Maybe apm when you would have wanted to run apm-dev, or something like that. If you want to use apm from the command-line, should run apm --version or apm-dev --version to troubleshoot. It will tell you the Atom it's building against. [Edit: This doesn't work on Windows until Atom/Atom Dev/Atom Beta/Atom Nightly is installed. I can't usually see any output when I'm running .\apm.cmd --version outside of a full Atom install. I think that's a bug in apm. apm --version or apm-dev --version usually works after installing Atom, though.])

DeeDeeG commented 4 years ago

The "Node.js 6.x or later" thats still on the Flight manual was probably also a "It's my sense that that isn't the case" recommendation,

This was true at the time it was written. The flight manual's build requirements just haven't been updated for a long, long time.

We are working on a PR for that. Just need to decide what phrasing to use or use what's already in the PR, get an approving review and a merge by upstream. Or... I'd really rather not fork the docs. Upstream is still alive. They need current docs, too.

jeff-hykin commented 4 years ago

Just need to decide what phrasing

I'd hope for something like: " The exact versions being used to build Atom can be found inside the package.json (link) under "pinnedBinaries". However, you likely don't need the exact versions. As of August 2020, the general requirements are:

"

@DeeDeeG

Sorry to keep saying this, but it's true every time I say it.

Without evidence, saying its true doesn't add to the conversation. Builds are not reliable, even if you want them to be, even if they should be. "If something breaks on other versions of system Node, it's a bug." Fine, lets label it a bug, that doesn't help me or other people build Atom.

If you want to label that Atom "should" work with ___, thats fine with me. But withholding a "We know for sure you can build Atom with these exact versions" makes it harder for people to contribute. What downside is there to adding that?

Docker was invented BECAUSE of this problem of reliability, its a serious industry issue inherent to all software. But, let me provide evidence; as part of the coding club at my university we take student projects of all kinds, help them get a team, and make the project reliable for all of them. Students come with all kinds of operating systems, preinstalled tools and environments because they're developers, just like us. These projects are fairly simple; static websites, mobile apps, python data plots, command line tools, and even some electron apps. Me and the rest of the senior team created and tested build scripts for weeks on VM's of multiple versions of Windows, Ubuntu, Manjaro, and laptops with the last 4 versions of MacOS. With the build script working in all of those environments, we gave a tutorial to a room of 60 people. Almost 20% had setup failures and issues of some kind.

But that wasn't an isolated event, this happened bi-weekly for years. It happened so often that I put together a team to build a toolkit called ATK (its on Github) for creating a reliable build scripts. I spent 2 years on the project, and after testing it in hundreds of different environments, it is still unreliable. There are so many hidden dependencies and interactions, reliability can't even be ensured even when exact versions are pinned.

I've spent way to much time working on the dependency problem, pinned versions help development. Its like booting atom in safe mode. Just because should work without safe mode doesn't mean safe mode isn't needed.

DeeDeeG commented 4 years ago

Long story short: I'm sorry for being difficult and for my tone. And I still want us to spend some effort understanding why the build failures you are seeing are happening, and why Node v12.4.0 and other pinned versions would help (please do file issues about it if you can spare the time/energy! I'm just sincerely trying to troubleshoot). But I'm willing to work on tracking down these build failure scenarios myself if no-one else wants to worry about it. And I don't personally understand very well which versions need pinning (I can't reproduce the conditions where pinned versions help), so I don't want to undertake writing that documentation myself. @aminya or @jeff-hykin please one of you handle pinning the versions. I'm not against it in the short-term, but in the long-term, I think understanding the specific build failure scenarios would also be a boon to this project.

(More thoughts: We can't stay on Node 12.4.0 forever if we want Electron 7 (Electron 8 is the oldest supported Electron!) and we will probably want to keep bumping npm... and so on. Not pinning means we are testing on a wider range of stuff and making sure it all works. That's my vision here. If we pin, nothing stops me personally from trying to keep it building on other versions, so maybe I'll just take that on as a personal task.)

Full ginormous reply (click to expand if you want): Sorry for the tone of my responses. > pinned versions help development If folks feel strongly about this, I won't try to stop them, and I don't mean to be annoying or disrespectful about it. My apologies. I should really cool down and be more measured with how I respond. > Builds are not reliable, even if you want them to be, even if they should be. "If something breaks on other versions of system Node, it's a bug." Fine, lets label it a bug, that doesn't help anyone contribute to Atom. I hear your frustration and I acknowledge you and @aminya reporting that this doesn't work reliably, if at all, for either of you. So it's two against one now, meaning my assertions that it "should work" aren't helping much, as you say. I'm squarely outnumbered as it pertains to anecdotal levels of evidence. What I should have emphasized is that this "works for me," which is a lower standard of proof than "always works." I don't mean "It always works," just "it should be working and let's find out why it's not." When I say "It's a bug" I'm very sincerely asking that you or @aminya file an issue here. Let's change it from "something isn't working but we're not sure exactly what it is" to "Okay we sort of know what's not working" or even "We know exactly what's not working"... best-case scenario we find the fix or the least-impactful workaround, and it helps us refine the documentation to be more helpful and accurate. "It's a bug" isn't meant to be dismissive, it's a call to action to post issues with whatever details you can find and let's track down the problem. I apologize again if it came off that way, because I was admittedly getting upset, and I could've been calmer. > Docker was invented BECAUSE of this problem of reliability, its a serious industry issue inherent to all software [...] Me and the rest of the senior team created and tested build scripts for weeks on VM's of multiple versions of Windows, Ubuntu, Manjaro, and laptops with the last 4 versions of MacOS. With the build script working in all of those environments, we gave a tutorial to a room of 60 people. Almost 20% would have setup failures and issues of some kind. [...] This isn't one tutorial failing, this was every 2 weeks for years. [...] I spent 2 years on the project, and after testing it in hundreds of different environments, it is still unreliable. Thanks for sharing that. I can say I hadn't known where you were coming as clearly before. I admire that project and I respect the experience and learning you've had. I don't know how to segue that into anything, but I don't want to dismiss anyone else's expertise. I too share a desire to have this project, and all open-source software, be accessible to the maximum number of people. My belief is that, long-term, knowing _why_ our dependency stack is what it is is more important than finding some versions that work in the now. Any old dependency could become unavailable or receive a patch-level bump that breaks the house of cards we've built. I want to understand our margins of safety to know how healthy or close-to-breaking our tech stack is. I don't want it to be a surprise when a build fails, I want us to know the answer of why. --- Pinning is okay, but still, if you've tried anything other than the pinned versions and it fails, please do send me the error messages you get, in some form or other. I want to track down the problems and fix them. I'm personally dedicated to diving in and understanding what frustrates our potential developers in as granular a level as possible and fixing it. > withholding a "We know for sure you can build Atom with these exact versions" will only make it harder for people to contribute. Why would you still be against adding that? I am cautious about it, because the evidence for it is anecdotal. What works for three people (Aminya, you and myself) may not work for the next person just because it's working for us. Node 12.4.0 isn't exactly the same as Electron 6. It has a separate API/ABI. Using it only closely approximates the Electron we are building for. Without any complete explanation for why only this version works, other than intuitively "it matches Electron" (when it shouldn't have to) is again a bit superstitious vs. factual. That shouldn't stop us from doing that informally, or saying "If you're having difficulty building Atom, the following versions are the most-tested and should work for you." The issues are just that pinning means you need to keep manually updating the pinned version (including in the docs). People forget to. An if we update the pinned version with any frequency, someone will inevitably land on outdated docs at some point and be confused why the exact version we suggested isn't working anymore. Non-pinned versions are a luxury I'm trying to preserve. They're the optimal situation, if actually workable. But I can admit it's proving controversial to suggest that that works, when two out of three people here can't use non-pinned versions, anecdotally. --- Bottom line: I am willing to be neutral toward using pinned versions. I won't stop anyone from doing it. If at all possible, I'd like us not to not simply lament the lack of evidence for or against, but rather compile more evidence so we can say definitively what's going on. Pinned versions are an acceptable workaround, but absent any proper explanation of why they might be needed, I continue to seek the missing explanations so we can fill in the gaps and understand the problem. If you folks are just wanting to move on, I'll have to do that research myself when I get the time. Otherwise join the effort, file bugs. I think that's all I should say about that. I don't understand intimately which pinned versions are needed, so @aminya or @jeff-hykin I would leave it to one of you to put together the documentation and so on about it.
DeeDeeG commented 4 years ago

I made #118 and #116 to start looking into this kind of dependency/build issues, to work out what's going wrong. I have a baseline "steps to reproduce" to reproducibly build on Node ^10.12.0 and Node 12 < 12.17, at least on my personal Linux machine. If anyone wants to post in there with some info for problems they ran into during builds, that would be great. I understand if people would rather not, or would like to spend their time doing something else with respect to this project, so just add something there if you feel like it.

Best Regards,

- DeeDeeG

jeff-hykin commented 4 years ago

Sorry for late reply @DeeDeeG, I got swamped with work later that day.

Long story short: I'm sorry for being difficult and for my tone.

Hey, I appreciate it. Don't worry about apologizing 👍 just promise when our roles are reversed (and I'm the one being difficult) that you'll be patient with me, and keep pushing your point. 😁

we will probably want to keep bumping npm... and so on

On a similar note to my last comment, there may have been some poor communication on my side on this original post.

I mean pinning in a way similar to a package-lock (auto generated), not at all in the way that Atom pins/freezes the version of Electron for a long period of time. I'm just talking about having a record: when any build is completed, the version of node used (whatever it might be) is saved, like echo "$(node -v)" to the package.json. The version of node could change with every build, but we'd have a record of exact versions if trying to reproduce it.

@aminya or @jeff-hykin please one of you handle pinning the versions.

No problem 👍 I'll happily work on this. I've got a bunch of tools for it. Assuming I can also get the Electron 6 version working, I'll probably have a PR next week.

You might also be more comforted by a system that lists multiple pinned versions. Basically have every successful build for a particular commit record the exact OS version along with all the versions. In the CI scripts we could iterate through versions of node/python/etc basically providing more and more proven options with each additional test.

And I don't personally understand very well which versions need pinning

All of them haha; git, node, npm, powershell, python, XCode, Visual Studio and anything else that is depended upon.

Any old dependency could become unavailable or receive a patch-level bump

Thats a good point, and maybe I wasn't initially clear. I think most everyone (certainly myself) will still use loose/unpinned versions for every day builds and testing: my node version practically changes with the sunrise, BUT whenever we get a build issue, we can compare ours to the pinned versions to start figuring out what's different/wrong. Rest assured though, I agree we should still treat failures on different versions like bugs and fix them. Fragility is bad, and I'd even like to work on making the electron version less locked-in. 👍

I am cautious about it, because the evidence for it is anecdotal. What works for three people (Aminya, you and myself) may not work for the next person just because it's working for us

True, I should have changed 'you' => 'we' for "We know for sure we can build Atom with these exact versions"

you need to keep manually updating the pinned version (including in the docs)

We can just have the docs reference the auto-generated record of the version so they're never out of date. (Or, even better, we could maybe auto-generate a change in the docs).

jeff-hykin commented 4 years ago

Node 12.4.0 isn't exactly the same as Electron 6

I didn't realize this was part of the problem. Is there a way to build it with Electron directly? (Similar to using the apm version does? @aminya)

I would want to use the node/electron inside the Atom app, except there needs to be a way to try to port Atom to new system like raspberry pi or the new ARM Macs. If there's a way to create builds for them without being on those systems then using the node/electron inside the Atom app seems viable.

aminya commented 4 years ago

I'll happily work on this. I've got a bunch of tools for it. Assuming I can also get the Electron 6 version working, I'll probably have a PR next week.

That would be awesome! I was thinking about how to approach this issue. But since you have done similar things before, it would help us a lot.

I didn't realize this was part of the problem. Is there a way to build it with Electron directly? (Similar to using the apm version does? @aminya)

That is not a problem. As stated in the docs, each Electron version uses a certain Node version. So running script/build or script/bootstrap using the same version, ensures compatibility.

If native modules are built with another version, and the build succeeds, in the end you can use apm rebuild or electron-rebuild to rebuild the native applications to match the Electron version. See this link for that: https://www.electronjs.org/docs/tutorial/using-native-node-modules You can also cross-compile by setting target_arch. I don't know which arch this supports though.

aminya commented 4 years ago

I think for the Node version I can write a script runner that runs the various scripts using the exact Node version that apm provides. That will solve all the Node issues. Minimizing the need for documentation is the best practice. The things should be automated as much as possible.

Probably downloading the exact Node exe from Node website is the easiest solution here.

We already have something for Windows x86. I just need to make this general.

We can also write the code in PowerShell/Bash, so the code works for anyone, even if they don't have Node installed.

P.S: turns out it is not just me and @jeff-hykin. @UziTech also has the same problem. So I feel this script runner is necessary.

DeeDeeG commented 4 years ago

Is there a way to build it with Electron directly?

Strictly speaking, Atom itself, and all packages/native code bundled with Atom, are built only against Electron.

The general case is as follows: apm sets a variety of env vars telling node-gyp (which is the main native code build system for the Node ecosystem) to build native code against an Electron target.

When running [Atom repo root]/script/bootstrap, the targeted version of Electron will be the one set as electronVersion in [Atom repo root]/package.json. When running apm as bundled with an Atom installed via an installer .exe or .deb, it will be the Electron version the Atom was built with. The actual full Electron binary isn't downloaded or used by apm, only the necessary headers and such. These headers are saved separate from any Electron binary and are only used by node-gyp, and only to build native code. There should not be any cross-contamination going on.

An aside (click to expand): _(To clarify, but please don't get caught up on this, it's WAY WAY WAY less important than it sounds: Apm bundles a version of node, which has one job: execute apm. That's it. There are some extremely rare cases where packages sniff this version of Node, so that's why we try to match it pretty close to the Electron version. Most packages do not crudely detect the version of Node to build against this way. It is a non-issue 99.999999% of the time. This is the very very rare exception, not the rule. For the details on this narrow case, see: https://github.com/atom/apm/issues/328. As far as I know, this is the only outright confirmed reason for pinning anything to an exact version that we have come across. And it's a corner case that it addresses, not the general case. It may not even be a valid workaround anymore, as Electron and Node ABI versions are now strictly separate, as of January 2019 (https://github.com/nodejs/TSC/issues/651).)_

As such, please be aware, manually running npm install, npm ci npm rebuild or any npm command to install modules in the root of the Atom repo is not supported and may cause breakages for later builds. (npm builds against Node headers by default, apm builds against Electron headers by default.) If you need to update some dependencies in the main package.json or package-lock.json, you can have apm handle it by running script/bootstrap. Or you can do npm install [package@semver-range] --package-lock-only to only update the package.json and package-lock.json, but not actually install anything.

Please also be aware: If a module (npm package) is already installed to node_modules, and you run npm install or apm install there, the native code will not be rebuilt. npm/apm see the module already installed and try to save time by not rebuilding it. To get fresh native code rebuilt, you need to run script/bootstrap --ci, or delete the module from node_modules before doing script/bootstrap. Or to be safe, if you don't know exactly which modules need to be rebuilt, delete the entire node_modules and start fresh with another script/bootstrap.

It's important to do it via script/bootstrap rather than manually running apm install, as the proper Electron version to build against is passed to apm in script/lib/run-apm-install.js (which is part of the bootstrap process). You can replicate those environment variables manually outside of the bootstrap script, I suppose, but why fuss with it? Manually intervening like that is hard to reproduce.

In summary: script/clean followed by script/bootstrap should be fairly reliable and solve a large number of problems.

aminya commented 4 years ago

The issue is worse than running clean before bootstrap. This issue appears even when a user wants to run some package in an already built Atom (e.g. here). That's why we should fix: https://github.com/atom-ide-community/apm/issues/13

Here is my WIP branch: https://github.com/atom-ide-community/apm/tree/electron-rebuild

DeeDeeG commented 4 years ago

This issue appears even when a user wants to run some package in an already built Atom

Right, I can see that would be a problem. Here's the rub: at some point, native code compiled for a specific version of Electron is saved to disk. Particularly saved to disk in [User's home folder]/.atom/, which is shared across any and all Atom versions.

Atom doesn't rebuild all packages on startup at the moment. That would be the real solution to these woes.*

* (With a large startup performance penalty, and the downside of added complexity. Maybe we could prompt the user after startup? Not sure if we can properly detect out-of-date packages, though, so the prompt would be at every startup... Maybe we should hide it in the menus instead of prompting every time... The current alternative is that users carefully double-check and ensure they've built their packages for the right Electron version.)

UziTech commented 4 years ago

Atom doesn't rebuild all packages on startup at the moment. That would be the real solution to these woes.

I usually have multiple instances of Atom open at once. I think if Atom rebuilt all packages on every reload it might cause issues with instances already running. (Not to mention probably make startup much slower)

I think the real solution is to have a .atom/beta and .atom/dev folder that would be used by beta and dev so they could truly be separate installations.

UziTech commented 4 years ago

To keep with the naming conventions the folders should probably be ~\.atom, ~\.atom-beta, and ~\.atom-dev

DeeDeeG commented 4 years ago

This is good for rapid development, but will still get hit with the issue if a user's stable install upgrades from Electron 5 to Electron 6. Or their beta install upgrades. And so on.

It's an improvement, but still could be exposed to this issue, albeit less frequently.

(Not saying we shouldn't do it even with that caveat. Just mentioning.)

UziTech commented 4 years ago

We could somehow check on startup if any modules were built using a different version of electron and then upgrade them. I don't think we have to worry about versions of electron changing frequently for normal users, but it could change frequently when switching between stable/beta/dev versions of Atom.

DeeDeeG commented 4 years ago

This (see link) is supposed to be the existing user interface for most of this stuff: https://github.com/atom-ide-community/atom/tree/master/packages/incompatible-packages#readme

I think we've found it to not be perfect, occasionally missing things (the julia-client package in particular, if I remember correctly?). But it's there!

aminya commented 4 years ago

For internal packages and building itself

We need the script runner to fix this. https://github.com/atom-ide-community/atom/issues/111#issuecomment-683769885

For external packages

Copying my comment from https://github.com/atom/apm/pull/889#issuecomment-672631876

One of the problems may be with the code that detects the incompatible package. The cache probably returns an incorrect result. For Hydrogen that I have installed, I got the incompatible error message. But I did not for julia-client.

https://github.com/atom/atom/blob/2d3e332b889bce1e458d154efa608016f47d8b9d/src/package.js#L1283

jeff-hykin commented 4 years ago

native code compiled for a specific version of Electron is saved to disk.

Yeah that sounds pretty bad (and difficult to fix). Also thanks for the details on the env and node-gyp stuff.

IMO we (as users) should be able to have multiple versions of Atom installed simultaneously even with different node versions. It might be tough to do, but I think (eventually) ~/.atom should only have config info. E.g. which extensions but not the entire extension itself.

jeff-hykin commented 4 years ago

Atom doesn't rebuild all packages on startup at the moment. That would be the real solution to these woes

We could somehow check on startup if any modules were built using a different version of electron and then upgrade them.

I agree. The version of node that the extension was built could be saved inside the extension's package.json.

This will also be helpful when users generally have issues with extensions. The check can be done asynchronously after Atom has already started, so it won't add much of a performance penalty. For normal users it should cover any issues they have from updating or somehow using multiple versions of Atom, since they're unlikely to bound between versions.

jeff-hykin commented 4 years ago

I think the real solution is to have a .atom/beta and .atom/dev folder that would be used by beta and dev so they could truly be separate installations.

I think this would be nice for testing so different configs could exist. If not too difficult I think we should explore this. However, I don't think this is a good solution for compiled dependencies

  1. It doesn't encourage forking of Atom. I want to have atom/atom, atom-ide-community/atom, and my own build of atom jeff-hykin/neodymium installed at the same time. If they're all looking at ~/.atom for compiled extensions that's going to be a nightmare. However, if they're only pulling in configs from ~/.atom that would be convenient.

  2. For multi-user installations, if two users use the same extension and same App, IMO the compiled extension should exist in one place, and simply loaded with different configs based on each user's ~/.atom. For direct extension it's not too much of an issue, but if extensions have dependency trees, a whole lot of dependencies could be shared instead of duplicated for each user.

UziTech commented 4 years ago

We could make it configurable. Something like atom --config-folder path/to/.atom

UziTech commented 4 years ago

It would also need to be configurable in apm

jeff-hykin commented 4 years ago

Also, just to clarify my position, if the compiled extensions are to be moved outside of ~/.atom, I don't think that change should be made anytime soon since its so major.

jeff-hykin commented 4 years ago

We could make it configurable. Something like atom --config-folder path/to/.atom

Maybe automatically use the name of the app (and still allow a --config-folder option). Atom Dev => ~/.atom-dev Neodymium => ~/.neodymium

This would solve my first point (forking), and my second point is low priority anyways

UziTech commented 4 years ago

I don't think that change should be made anytime soon since its so major.

I don't think it would be a breaking change if we made it configurable as long as we keep the default the same.

aminya commented 4 years ago

Instead of using different folders, we should first fix this so Atom detects the issue and recompiles things: https://github.com/atom-ide-community/atom/issues/111#issuecomment-683974477

Then we can work on optimizing by using different folders such that no recompilation is needed for different Atom versions. IMO, the folder name should be based on the Electron version which affects the native module recompilation: atom-e6 or atom-e5.

jeff-hykin commented 4 years ago

Instead of using different folders, we should first fix this so Atom detects the issue and recompiles things: #111 (comment)

I agree, lets focus on recording and checking the versions and worry about multiple Atom versions in a separate issue.

I built a tool and published it on npm for tracking versions of binaries, and I'm working on a PR now. My build still fails with node v12.18.3 but I installed exactly 10.20.1 to match the apm version and that worked (without using the one inside apm).

jeff-hykin commented 4 years ago

PR created: https://github.com/atom-ide-community/atom/pull/134

jeff-hykin commented 4 years ago

Note, maybe a new issue should be made for this:

(Look at both node versions) This was the successful build at the top of this thread 91370171-40d46b00-e7d3-11ea-93cb-178b2fca13dc

This was from my successful build with latest master Screen Shot 2020-09-05 at 3 47 32 PM

🤔 🤔 🤔 🤔 🤔 It looks like something really jank is happening.

DeeDeeG commented 4 years ago

Hi @jeff-hykin that's a great question to ask. I happen to have the answer.

The first set of versions are what you have on your system PATH.

These are printed by script/lib/verify-machine-requirements. That script has checks geared toward making sure you can successfully install and execute the build scripts. (If verify-machine-requirements.js errors out, it's because the versions of Node, npm, and/or Python on your system are inadequate to complete the build). That's what these requirements are for: installing and running the scripts. (Not directly used for building native code for Atom! Just used to run the scripts!).

verify-machine-requirements.js output, explained line by line (click to expand): - Node (on your PATH) - Npm (on your PATH) (Note: If you have already installed the `script/node_modules` dependencies before running `script/bootstrap`, this will print the version of `npm` located at `script/node_modules/.bin/npm[.cmd]`) - Python (on your PATH, or else somewhere on the system that `node-gyp` would be able to find)

The second set of versions are printed by apm --version. They are the versions relevant to building Atom itself. A good amount of these are bundled with apm itself.

apm --version output, explained line by line (click to expand): - apm (The version of `apm` you are using to execute the `apm --version` command) - npm (This is a separate copy of `npm` bundled and included with `apm`. `apm` is a wrapper around this version of `npm`.) - node (The version of Node bundled with `apm`. This is a separate copy of a Node binary that will have one job: execute `apm` commands.) - atom (This is whatever version of Atom is installed to your system. If it matches the Atom you're running `script/bootstrap` for, that's just a coincidence. This field is much more sensible after you install Atom. For a properly installed full Atom release, this prints the Atom version that the version of `apm` you are running was bundled with.) - _Note: This "atom version" is pretty much meaningless during `script/bootstrap`. Even if you haven't installed any copies of Atom system-wide, it will print "unknown" and move on just fine with the build._ - python (The version of python `apm` could find on the PATH. Note: `apm` isn't good at printing the version number of binaries called `python3` at the moment. But `node-gyp` will still use Python 3 just fine under the hood. Don't worry if no Python version is listed here.) - git (The version of `git` that `apm` could find on the PATH.)

In conclusion: Nothing too janky. This is all running as expected.

Maybe we should print "System Dependencies:" for the first set of versions, and then "apm Dependencies:" for the second set, to make this less confusing. I admit it could be made clearer at a glance what's going on.