Alternative build method #420

mauricioszabo · 2022-07-03T15:21:12Z

I decided to draft this PR to request for comments (I think "code' speaks better than words in this case). So, this PR fundamentally changes how we create Atom binaries.

Motivation

Currently, Atom builds binaries with a combination of ./script/bootstrap and ./script/build. I could not run ./script/boostrap in a reliable way, and discussing with some people it seems that it does not function with recent npm versions. Also, all the scripts to build Atom end up adding more than 5.000 lines of code that the community will have to both understand and maintain, so I decided to make a quick spike to see how hard it would be to build Atom without these scripts

How is being done?

I'm using yarn instead of npm and I'm adding electron-builder, Electron and some other packages to dev-dependencies for easier bootrapping of the app. I also updated a doc to show how to start developing Atom with this method.

What is not working?

Unfortunately, this method is not 100% complete yet. Atom works, but github package is developed with ESM modules, so it can't be loaded as of now (probably the boostrap script ends up using babel to transpile code). Other packages work, but another problem is that "core" packages are missing their README files for some reason. So the list of things I know we'll have to take a look:

Github package is not working
README of "core" packages is also not working
When we start Atom, for some reason it locks the terminal - as if we're always running Atom with the -f switch
V8 Snapshots doesn't currently work - seems that they also need Babel to transpile packages
apm does not get installed on the PATH - this may need to be fixed

What is the advantage?

Updating some "core" packages like Electron and Tree-Sitter currently is very hard. Atom solution is to "boostrap" and then point an existing Atom editor to the "bootstrapped" package, which is not that reliable and also does not help on the Electron case. Also, it's all made with custom scripts, which are not reliable as they are now. We may fix the scripts to work 100% of the time, but IMHO it's better to embrace what the Node/JS/Electron community is using so we gain bugfixes and access to more targets when they appear.

Also, the current package is able to auto-update an AppImage on Linux if we decide to implement this (electron-builder have a way to implement a cross-platform independent auto-updater). It is also able to draft releases on GitHub too, and the number of lines of code that I had to write to make it work is really small compared with the scripts directory.

Should we decide to go through this route, we have to decide if we're going to keep the idea of using "V8 snapshots" to load Atom faster, and if we want to transpile code. Also, maybe it's a good idea to, in the future, change the way Atom detects "core" packages - currently, it's somewhat weird and depends on adding stuff on package.json. Maybe a better way could be to just make Atom search for packages in two directories - the ATOM_HOME and the "asar" file when bundled.

mauricioszabo · 2022-07-03T19:02:37Z

Forgot to mention: I want comments if people think this is a move in the right direction. If you think so, I'll work to fix the issues I mentioned on this PR :)

aminya

I like the approach to modernizing the build script. 👍🏻

However, I am not sure if this covers all the things that the current build script does. So it needs some close comparison and verification.

Some things don't match how we have been working in the organization. For example, we have been trying to use pnpm instead of apm for years. So, this should change yarn back to apm/npm (since the pnpm conversion is happening on other branches).

Should we decide to go through this route, we have to decide if we're going to keep the idea of using "V8 snapshots" to load Atom faster, and if we want to transpile code.

We definitely need to keep the V8 snapshots. The performance difference is like night and day. We also need to keep transpilation as many projects rely on this.

Once this is ready, we should probably remove the old method. We don't want to maintain two parallel ways to build Atom.

mauricioszabo · 2022-07-04T14:34:20Z

I like the approach to modernizing the build script. 👍🏻

Good, so I'll keep working on it. What I know is that it currently is not making any transpilation and it's not generating the V8 snapshots, so I'll try to make these work and update this PR :)

mauricioszabo · 2022-07-04T14:39:39Z

Some things don't match how we have been working in the organization. For example, we have been trying to use pnpm instead of apm for years. So, this should change yarn back to apm/npm (since the pnpm conversion is happening on other branches).

Honestly, I don't want to keep using yarn - if possible, I would like to mibrate to pnpm but that's what it is currently working. I'll revert back to pnpm (or even npm) when that works reliably, the choice for yarn was because it literally was the only thing that worked 100% of the time...

aminya · 2022-07-04T23:05:55Z

I changed the priority to low as it is not urgent.

DeeDeeG · 2022-07-07T03:40:33Z

Notes on the things that aren't working:

Github package is not working

github package should work if transpiled properly.

That is currently handled by script/lib/transpile-packages-with-custom-transpiler-paths.js

See also upstream's version of the same file: https://github.com/atom/atom/blob/master/script/lib/transpile-packages-with-custom-transpiler-paths.js)

When we start Atom, for some reason it locks the terminal - as if we're always running Atom with the -f switch

Well, normally atom is run from a shell script on Unix-like OSes. The shell script hands off to the real atom binary without occupying/blocking the terminal it was started from. You are probably calling the atom binary directly now? Which is just a renamed electron binary.

V8 Snapshots doesn't currently work - seems that they also need Babel to transpile packages

The snapshots are giving us relatively a lot of trouble whenever we try to change things.

Meanwhile, I can't really tell what difference it makes. I tried removing /usr/share/atom/snapshot_blob.bin from my Atom install dir on Linux, and it didn't seem to make a difference, or maybe loaded faster?? There may be bits and pieces of the snapshot in multiple places. I also think some of this "snapshotting" is re-compiled when you start Atom or install packages. So bundling a startup snapshot with Atom may really not be worth the effort.

Take a look at upstream's PR and reasons for introducing snapshots: atom#13916

The PRs that introduced the snapshot way back when contemplated speedups of ~200-300 milliseconds on loading a new editor, maybe around a second or two if loading an editor window with lots of packages to initialize. That's not much faster in my opinion, in order to deal with all this hassle of snapshotting. It's a nice idea, but causes so many headaches. We don't have the dev power upstream had at the time when they committed to snapshots.

HOWEVER: We can try excluding problematic paths from the snapshot script, rather than deleting the snapshot altogether... https://github.com/atom/atom/blob/v1.60.0/script/lib/generate-startup-snapshot.js#L31-L262

apm does not get installed on the PATH - this may need to be fixed

Probably because the relative path to apm is changed on this branch. I think if you installed with npm install --global-style instead of yarn install, and only copied over apm/node_modules/atom-package-manager as apm, it would fix things by putting apm in the same location the current build scripts expect apm to be.

mauricioszabo · 2022-07-07T15:58:30Z

@DeeDeeG using electron-builder it actually generates a single binary that is installed somewhere on the PATH. If we configure the build process to install additional "scripts" or "binaries" instead of what electron-builder generates, both apm and an atom that does not block the terminal should work. Again, I drafted this PR to know if this approach is interesting for people, and I do agree it's low-priority :)

aminya · 2022-07-08T04:59:27Z

package.json

+  "devDependencies": {
+    "electron": "11.5.0",
+    "electron-builder": "^23.1.0",
+    "electron-rebuild": "^3.2.7"
  }


These are going to break the snapshot script. They should be moved to script/package.json

aminya · 2022-07-08T05:00:35Z

package.json

+    "build": "electron-rebuild",
+    "build:apm": "cd apm; yarn install; ./node_modules/atom-package-manager/bin/npm rebuild",
+    "start": "electron .",
+    "dist": "node script/electron-builder.js"


I'd rather see the electron-builder as a part of the current build process. Then, the duplicate parts of the current build can be removed. See #311 for an example to run this. I did it for Parcel before generating the snapshots.

I actually made this experiment to see if we could replace the current build process, as it seems completely unreliable. But I also see your point. Maybe we can discuss this here? #443

I added my response here:
#443 (comment)

aminya

Added more comments in case we get back to this. But do not spend a lot of time on it for now.

aminya · 2022-07-10T07:45:27Z

script/electron-builder.js

@@ -0,0 +1,104 @@
+const path = require('path')
+const normalizePackageData = require('normalize-package-data');
+const fs = require("fs/promises");


fs/promises is only available on newer Node versions.

aminya · 2022-07-10T07:46:40Z

script/electron-builder.js

+  })
+}
+
+main()


All the promises should be handled.

Suggested change

main()

main().catch((err) => {

throw err;

}

aminya · 2022-07-10T07:47:09Z

script/electron-builder.js

+  builder.build({
+    config: options
+  }).then((result) => {
+    console.log("Built binaries")
+    fs.mkdir('binaries').catch(() => "")
+    Promise.all(result.map(r => fs.copyFile(r, path.join('binaries', path.basename(r)))))
+  }).catch((error) => {
+    console.error("Error building binaries")
+    console.error(error)
+    process.exit(1)
+  })


Please use async/await and try/catch

aminya · 2022-07-10T07:47:38Z

script/electron-builder.js

+    config: options
+  }).then((result) => {
+    console.log("Built binaries")
+    fs.mkdir('binaries').catch(() => "")


Why do you ignore the error?

aminya · 2022-07-10T07:48:04Z

script/electron-builder.js

+  }).then((result) => {
+    console.log("Built binaries")
+    fs.mkdir('binaries').catch(() => "")
+    Promise.all(result.map(r => fs.copyFile(r, path.join('binaries', path.basename(r)))))


All the promises should be handled.

Suggested change

Promise.all(result.map(r => fs.copyFile(r, path.join('binaries', path.basename(r)))))

await Promise.all(result.map(r => fs.copyFile(r, path.join('binaries', path.basename(r)))))

aminya · 2022-07-10T07:52:29Z

script/generate-metadata-for-builder.js

+function buildBundledPackagesMetadata(packageJSON) {
+  const packages = {};
+  for (let packageName of Object.keys(packageJSON.packageDependencies)) {
+    const packagePath = path.join('node_modules', packageName);


Please use CONFIG to get the absolute path of node_modules

aminya · 2022-07-10T07:56:02Z

script/generate-metadata-for-builder.js

+    const packageMenusPath = path.join(packagePath, 'menus');
+    if (fs.existsSync(packageMenusPath)) {
+      for (let packageMenuName of fs.readdirSync(packageMenusPath)) {
+        const packageMenuPath = path.join(packageMenusPath, packageMenuName);
+        if (
+          packageMenuPath.endsWith('.cson') ||
+          packageMenuPath.endsWith('.json')
+        ) {
+          packageNewMetadata.menus[packageMenuPath] = CSON.readFileSync(
+            packageMenuPath
+          );
+        }
+      }
+    }


Each of these blocks of code can become a function with a descriptive name and good documentation.

aminya · 2022-07-10T07:58:10Z

package.json

@@ -1,5 +1,6 @@
 {
  "name": "atom",
+  "author": "Atom Community <[email protected]>",


The correct email is [email protected]

aminya · 2022-07-10T07:58:31Z

package.json

@@ -161,7 +161,7 @@
    "temp": "0.9.2",
    "text-buffer": "^13.18.5",
    "timecop": "https://www.atom.io/api/packages/timecop/versions/0.36.2/tarball",
-    "tree-sitter": "git+https://github.com/DeeDeeG/node-tree-sitter.git#bb298eaae66e0c4f11908cb6209f3e141884e88e",
+    "tree-sitter": "^0.17.2",


Should be a different pull request

aminya · 2022-07-10T07:58:49Z

package.json

@@ -69,7 +70,6 @@
    "fuzzy-finder": "https://www.atom.io/api/packages/fuzzy-finder/versions/1.14.3/tarball",
    "git-diff": "file:packages/git-diff",
    "git-utils": "5.7.1",
-    "github": "https://www.atom.io/api/packages/github/versions/0.36.10/tarball",


Why do you remove GitHub from Atom?

mauricioszabo · 2022-07-11T14:11:55Z

Ok, so this PR was to try an alternative build method that did not depend on scripts at all (or the least we can). The way it's currently moving, and considering priorities, this is probably add another moving piece to what is somewhat too many moving pieces, so I'll close this PR. We may re-organize ideas in the future, when higher priorities are attended.

Also, to handle the new requirements of using electron-builder over the transpiled version of packages and everything, this PR will need to change quite a lot; finally, as we're now, I'm monkey-patching Electron Builder to not remove some metadata that we actually need, so again, I'm not sure this is useful right now.

So, closing for now as to not clutter the PRs we actually need to attend, we may re-visit this in the future.

aminya · 2022-07-12T20:51:55Z

Please keep it open so it is not lost

mauricioszabo added 3 commits July 3, 2022 11:41

Added scripts and removed "github"

6c9341a

Script to package with Electron-Builder

e2b9296

Adding info on Building

f2d22ac

Adding lockfiles

309971b

aminya requested changes Jul 4, 2022

View reviewed changes

aminya added experiment medium-priority labels Jul 4, 2022

aminya added low-priority and removed medium-priority labels Jul 4, 2022

aminya reviewed Jul 8, 2022

View reviewed changes

aminya requested changes Jul 8, 2022

View reviewed changes

aminya requested changes Jul 10, 2022

View reviewed changes

mauricioszabo closed this Jul 11, 2022

aminya reopened this Jul 12, 2022

ghost closed this by deleting the head repository Oct 6, 2022

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Alternative build method #420

Alternative build method #420

mauricioszabo commented Jul 3, 2022

mauricioszabo commented Jul 3, 2022

aminya left a comment •

edited

Loading

mauricioszabo commented Jul 4, 2022

mauricioszabo commented Jul 4, 2022

aminya commented Jul 4, 2022

DeeDeeG commented Jul 7, 2022 •

edited

Loading

mauricioszabo commented Jul 7, 2022

aminya Jul 8, 2022

aminya Jul 8, 2022

mauricioszabo Jul 9, 2022

aminya Jul 9, 2022

aminya left a comment •

edited

Loading

aminya Jul 10, 2022

aminya Jul 10, 2022

aminya Jul 10, 2022

aminya Jul 10, 2022

aminya Jul 10, 2022

aminya Jul 10, 2022

aminya Jul 10, 2022

aminya Jul 10, 2022

aminya Jul 10, 2022

aminya Jul 10, 2022 •

edited

Loading

mauricioszabo commented Jul 11, 2022

aminya commented Jul 12, 2022

+                })
+              }
+              main()

-main()
+main().catch((err) => {
+  throw err;
+}

	Promise.all(result.map(r => fs.copyFile(r, path.join('binaries', path.basename(r)))))
	await Promise.all(result.map(r => fs.copyFile(r, path.join('binaries', path.basename(r)))))

Alternative build method #420

Alternative build method #420

Conversation

mauricioszabo commented Jul 3, 2022

Motivation

How is being done?

What is not working?

What is the advantage?

mauricioszabo commented Jul 3, 2022

aminya left a comment • edited Loading

Choose a reason for hiding this comment

mauricioszabo commented Jul 4, 2022

mauricioszabo commented Jul 4, 2022

aminya commented Jul 4, 2022

DeeDeeG commented Jul 7, 2022 • edited Loading

mauricioszabo commented Jul 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aminya left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aminya Jul 10, 2022 • edited Loading

Choose a reason for hiding this comment

mauricioszabo commented Jul 11, 2022

aminya commented Jul 12, 2022

aminya left a comment •

edited

Loading

DeeDeeG commented Jul 7, 2022 •

edited

Loading

aminya left a comment •

edited

Loading

aminya Jul 10, 2022 •

edited

Loading