WIP: Add version-tracker #134

jeff-hykin · 2020-09-05T20:44:55Z

This is all of the executables I'm aware of

node
npm
xcode
clang
Visual Studio
apm
powershell
python
python2
python3
zsh
bash
git

Visual Studio command still needs to be added, but that can be done after the PR

aminya · 2020-09-05T20:59:33Z

Does linux need gcc/clang?

Yes. Linux uses Clang 9. See this: https://github.com/atom-ide-community/atom/blob/master/script/vsts/platforms/templates/preparation.yml

aminya · 2020-09-05T21:06:02Z

I would recommend making a backup of this branch and reset --hard against our master, and then cherry-pick the commits from your backup. Sorry about this. I just updated our master now.

@jeff-hykin

aminya · 2020-09-05T21:13:40Z

package.json

+      {
+        "name": "visual studio",
+        "versionCommand": [
+          "C:\\Program Files (x86)\\Microsoft Visual Studio\\Installer\\vswhere.exe"


vswhere does not give the version of the installed visual studio.

Do you know of a global command for Visual Studio?

Yes, but it is not that straight forward.

vswhere -products * -find **/Hostx64/x64/*

gives the list of all installed visual studio installations. Then we should find 2017 from the given path.

OK here is a one liner in PowerShell

❯ (vswhere -products * -find **/Hostx86/x86/* | select-string '(\\MSVC\\)(.*)(\\bin)').matches.groups[2].value 14.16.27023

Works for both x64 and x86

Thanks! that helps me out a ton.

Do you install vswhere in your script? Since this is not available on Windows by default.

jeff-hykin · 2020-09-05T21:19:16Z

I would recommend making a backup of this branch and reset --hard against our master, and then cherry-pick the commits from your backup. Sorry about this. I just updated our master now.

@jeff-hykin

Nothing like a good force push to fix everything haha

DeeDeeG · 2020-09-07T14:28:58Z

Can we log the results in another file than package.json? successful-builds.json or version-tracker.json?

And perhaps we can set the configuration for this in a file other than package.json as well. It could all go in a version-tracker.json. Let me know what you think @jeff-hykin @aminya.

DeeDeeG · 2020-09-07T14:30:56Z

script/build

+    }).then(()=>{
+        // record the versions of binaries that were used to create the successful build
+        generatePackageJsonWithTracking()


If we do this in script/build, it will update package.json and invalidate all the caches. It's tricky for our CI.

See my comment above in this thread, but to reiterate: Can this config/logging go in a file other than package.json?

jeff-hykin · 2020-09-08T11:59:20Z

Yeah @DeeDeeG I'll make a new file for it. Does versions-log.json sound like a good name?

I prefer having all config info (babel config, browser list, etc) in the package.json. But that's just a preference, so I'm fine to put also put the version-tracker config into to the versions.log.

For merges and CI though I do want to point out; I think it makes sense to split up the config and the log though. Basically changes to the version log are always ignored because they're fully auto-generated, but changed to the config (e.g. package.json) would be hand-done (only updated if binary dependencies were added/removed)

jeff-hykin · 2020-09-08T12:18:31Z

Also FYI It'll probably be Sunday before I update the PR.

DeeDeeG · 2020-09-10T17:44:05Z

Also FYI It'll probably be Sunday before I update the PR.

No problem!

Does versions-log.json sound like a good name?

That's fine. It can be anything. If you have something else in mind that would work well if your module is used for lots of projects, I don't mind what you call it.

changes to the version log are always ignored because they're fully auto-generated

I don't quite understand? If it's written to disk, git will see the change, and folks will tend to accidentally commit it in their PRs. Maybe we can add the log.json file to .gitignore. That way folks don't accidentally add it in every PR. And we can still deliberately update it when we want to with git add -f versions-log.json or whatever the log filename is.

I think it makes sense to split up the config and the log

Fine by me. Given that they're both in package.json now, I thought it made just as much sense as that to put the config and logging together in a versions-log.json file of some sort.

As a design philosophy thing: I personally think if the logged versions are read by our code and used for something by our code then they could logically go in package.json. (If the version info is strictly logging information for humans to read, then it should go in a separate log.json file (whatever the filename).) package.json should only be info that's useful to our build scripts and/or useful to the running Atom app, or metadata npm can use to install our app (package dependencies and such).

This seems like info that's only relevant when developing and maybe when building (for example: if we follow up after this by somehow installing these exact versions in CI), so it should ideally not be shipped with the app if possible; package.json is shipped with Atom. We don't need build/CI metadata shipped with Atom. (Kind of like how the babel config is its own file. The linter config is its own file. We don't ship those with Atom.) That being my personal opinion.

Recapping the caching issue: If logging output has to be in package.json, it can't really be .gitignored and it can mess with caching, so I hope we can at least have the logged output in a different place than package.json.

jeff-hykin · 2020-09-12T00:47:32Z

Sounds good, thats good philosophy to know about for the package.json. And sorry about the "ignore" confusion. I only meant humans basically ignoring it in PR's, not gitignore. I think is should be auto-generated with every build, but not every PR. I'll add a limiter to the length so it doesn't become absurdly long (it'll delete old build info when new ones roll in). It'll just be like the package-lock.json where nobody pays attention to it except for special debugging cases.

aminya · 2020-09-12T01:18:53Z

I have a tool for the files that should be ignored most of the time but should be committed in some cases.
https://github.com/aminya/build-commit

I think this version-log is the same. We can commit it only in our release CI. That's what the organization uses for using TypeScript, Parcel, etc in Atom packages.
https://github.com/atom-ide-community/atom-ide-datatip/blob/fe61eec0db91901ef85f2f3eb42c27ae3820c5b1/.github/workflows/ci.yml#L74

aminya · 2020-09-16T06:38:42Z

@jeff-hykin I cannot push to your branch. If you can rebase this. In package.json ripgrep is updated. About the lock just accept the changes and bootstrap again.

jeff-hykin · 2020-09-17T12:31:16Z

@aminya I'm having some trouble with windows.

Any idea why spawnSync() wouldn't work on windows?

const { spawnSync } = require('child_process')
console.log(spawnSync("echo", ["hi"])) // fails

I was just going to test if node would automatically convert ./script/get-visual-studio-verion.ps1 to .\script\get-visual-studio-verion.ps1 since it does that for other operations, but then I realized the file doesn't execute at all.

Here's the more general thing I'm trying to get working

I've got a powershell script that runs fine

> .\Desktop\file.ps1
hello world

But it fails when running it in node

const { spawnSync } = require('child_process')
const { existsSync } = require("fs")

console.log(existsSync(".\Desktop\file.ps1")) // true
let result = spawnSync(".\Desktop\file.ps1",[])
console.log(result) // non-descript failure

DeeDeeG · 2020-09-17T14:33:52Z

child_process.spawn[Sync]() is a bit different from child_process.exec[Sync]() or child_process.execFile[Sync]().

https://nodejs.org/api/child_process.html#child_process_spawning_bat_and_cmd_files_on_windows

child_process.spawn[Sync]() runs outside of a shell by default.

I think "echo" might be a shell utility from cmd.exe or powershell. Likewise, auto-running scripts when "executing them" might be a nice thing the shell does for you.

So you might have to provide the shell as the command, then have your script be an argument to that command. cmd.exe [script here] or powershell [script here] or pwsh [script here]. Or try cp.exec[Sync]() or cp.execFile[Sync]().

aminya · 2020-09-18T00:59:08Z

console.log(spawnSync("echo", ["hi"])) // fails

echo is not a binary or file in Windows while it is in Linux. I recommend using shelljs which works independently from the operating system. Its CLI app is called shx which we use everywhere in our organization to write our build scripts.

Example of shelljs
Example of shx

Otherwise, you can spawn a shell instead specifically (like PowerShell or cmd).

jeff-hykin · 2020-09-18T05:18:37Z

Thanks for the input

child_process.spawnSync is a bit different from child_process.execSync or child_process.execFileSync.

Thanks to python2 -v returning the output in stderr, I actually switched to spawn sync (from exec) to intentionally to handle the stderr in an OS-independent way.

I like shelljs (actually last time I used it I think it was in an alpha/beta state), but I was hoping not to have it as a dependency for such a simple version tracking tool. I may end up needing it though.

echo is not a binary or file in Windows while it is in Linux.

Thanks. I was aware most the short powershell commands (echo, pwd, ls) were aliases, but I thought it was linking to a cmd executable. I did a lot of this stuff in the past (using shelljs) but I guess its been about 4 years. I was thinking Microsoft added execution support for powershell, but it looks like spawns only works for .exe and .bat files.

Sadly, while node converts paths for things like existsSync("./Desktop/file.bat") I confirmed just now that it doesn't for execFile and spawn . So I'm going to have to figure out another solution for cross platform version tracking. I think I'm just going to manually check the first argument and if its a file, convert as needed to the OS specific file format. I'll be looking into other solutions too, I was just hoping to avoid an "if windows do __" kind of statement in the package.json.

aminya · 2020-09-18T05:42:09Z

Why don't you use shelljs as I mentioned? It is a cross-platform solution for writing shell applications.

There is also this WIP node-shell, but I prefer shell.js which does not rely on Bash or Powershell.
https://github.com/rannn505/node-powershell/tree/node-shell/examples

If you can point to a specific code in your repository, I can help you with that.

Sadly, while node converts paths for things like existsSync("./Desktop/file.bat") I confirmed just now that it doesn't for execFile and spawn

For paths, you can use path.join. However, both CMD and PowerShell (at least in Windows 10) support / for paths.

cmd:

powershell:

aminya · 2020-09-18T08:01:02Z

Here I found another great package for execution: https://github.com/sindresorhus/execa

jeff-hykin · 2020-09-19T04:46:32Z

Why don't you use shelljs as I mentioned?

I like shelljs, but I was hoping not to have it as a dependency

^
(or any dependencies) it would go from being a package 1 file with 100 lines to (effectively) an entire 0.2Mb of JavaScript code. If there's no dependencies I also don't have to worry about updating it when exploits are found in common dependencies.

both CMD and PowerShell

Cool! I never knew. Sadly it still fails for spawn since I believe it doesn't invoke a shell

aminya · 2020-10-02T02:05:19Z

I have made the build script parallel + async in other pull requests. So, I moved the version tracker call:
https://github.com/atom-ide-community/atom/pull/134/files#diff-1293c4cc4b2f3a632b921bd168da9a8eR198

package-lock needs to be updated.

jeff-hykin · 2020-10-14T11:31:48Z

Version tracker was updated to handle issues (like powershell, and pwsh both being powershell), this branch was updated but I ran out of time so I'll look at the updating the package-lock and moving the version-tracker call next chance.

jeff-hykin · 2020-10-14T14:39:38Z

Okay, PR should be ready for review. @aminya I'm not sure what you mean by moving the version tracker call, it looks like its in the same place to me in that linked code.

package-lock was updated. Last step I need to do is test the build on mac/windows (Not sure when I'll get that done)

aminya · 2020-10-16T00:54:37Z

Awesome! I'll review and merge this soon.

aminya · 2020-10-19T21:46:00Z

script/get-visual-studio-verion.ps1

@@ -0,0 +1 @@
+(vswhere -products * -find **/Hostx86/x86/* | select-string '(\\MSVC\\)(.*)(\\bin)').matches.groups[2].value


This requires vswhere which may not be installed. I was recently involved with a project that uses some manual code to find Visual Studio in case vswhere is not installed.

https://github.com/ilammy/msvc-dev-cmd/blob/1cccac6a8dab508b878aeb0f56ff9b2ca774d201/index.js#L52-L64.

Another possibility is to bundle vswhere with version tracker (or Atom itself). It is a tiny exe file:
https://github.com/microsoft/vswhere/releases/download/2.8.4/vswhere.exe

dang, all that is needed just to check the version? 😬 I guess storing the exe in the atom repo is the best option

I'd suggest that you add this to the version tracker itself. It makes sense that you right a little script that downloads it and runs it. That would be useful for anyone who uses the version tracker (not just Atom).

Here is a sketch. Should I make a PR??

const fs = require('fs'); const path = require('path'); const download = require('download'); const childProcess = require('child_process'); const vswhereVersion = '2.8.4'; const PROGRAM_FILES_X86 = process.env['ProgramFiles(x86)'] || 'ProgramFiles(x86)' const vswhereDir = `${PROGRAM_FILES_X86}\\Microsoft Visual Studio\\Installer` const vswherePath = `${vswhereDir}\\vswhere.exe` async function downloadVsWhere() { if (!fs.existsSync(vswherePath)) { await download( `https://github.com/microsoft/vswhere/releases/download/${vswhereVersion}/vswhere.exe`, vswherePath, ); } } function findWithVswhere(pattern) { // if already installed and on PATH try { let installationPath = child_process.execSync(`vswhere -products * -latest -prerelease -property installationPath`).toString().trim() return installationPath + '\\' + pattern } catch (e) { console.warn(`vswhere failed: ${e}`) } // download await downloadVsWhere() try { let installationPath = child_process.execSync(`${vswherePath} -products * -latest -prerelease -property installationPath`).toString().trim() return installationPath + '\\' + pattern } catch (e) { console.warn(`vswhere failed: ${e}`) } return null }

I can make another repo that wraps that up, but I don't want to hardcode anything for version-tracker. I'm looking for it to be a one-and-done generic tool; no maintenance, security updates, urls that can go out-of-date, or broken node dependencies. Which is also why I didn't use shelljs

Actually it might be best to just wrap up vs-where and make it a commandline npm package.

how can node use Visual Studio without knowing what version it is?

I also kinda think the version tracking would be useful as-is (its just a debugging tool for people who build atom). We can mention the vswhere check+download in the docs, and create an issue to keep track of it, but the optional dependency of Visual Studio is kinda blocking/slowing an otherwise straightforward improvement.

If vswhere is included in all Visual Studio versions after 2017, I say we just require Visual Studio ≥ 2017

aminya · 2020-10-19T21:46:47Z

package.json

+          [
+            "pwsh",
+            ".\\script\\get-visual-studio-verion.ps1"
+          ],


Why do have two versions of this? The forward slash one is enough for all the operating systems.

If its tested and works without the extra's that would be great. The code is still using spawn which doesn't invoke a shell, but I guess since its calling powershell the argument will get converted

https://github.com/jeff-hykin/atom

…nto ide

…jeff-master

jeff-hykin mentioned this pull request Sep 5, 2020

Executable Versions + Script Runner #111

Open

add version-tracker

8cf6d0c

aminya reviewed Sep 5, 2020

View reviewed changes

jeff-hykin force-pushed the master branch from 827103a to 8cf6d0c Compare September 5, 2020 21:16

- remove visual studio

060cef9

DeeDeeG reviewed Sep 7, 2020

View reviewed changes

aminya force-pushed the master branch from 3d5a17c to 36aab2b Compare September 16, 2020 08:24

jeff-hykin added 4 commits September 16, 2020 07:30

- update version tracker

6f7a399

-

be65d41

- fix get-visual-studio-verion

01a029a

Merge branch 'master' into jeff

83fd959

aminya force-pushed the master branch 2 times, most recently from 87c664d to 67dcaef Compare September 18, 2020 12:24

aminya force-pushed the master branch 2 times, most recently from d5b2b31 to 1808785 Compare October 2, 2020 00:10

Merge branch 'master' into master

33bf89f

- update to the new version-tracker

dec3668

update package json

39fdfd0

- remove semicolons

6b322a0

aminya self-assigned this Oct 16, 2020

aminya added this to In progress in Build, bootstrap, CI via automation Oct 16, 2020

aminya added the dependencies Pull requests that update a dependency file label Oct 16, 2020

aminya reviewed Oct 19, 2020

View reviewed changes

jeff-hykin added 6 commits October 20, 2020 11:16

Merge branch 'master' of

e4abf98

https://github.com/jeff-hykin/atom

Merge branch 'master' of https://github.com/atom-ide-community/atom i…

85025ef

…nto ide

track os, remove redundant vs commands

8e33a16

fix bad merge

8f73602

add nix

6d11c39

fix nix

a30b2cb

aminya closed this Jul 22, 2021

Build, bootstrap, CI automation moved this from In progress to Done Jul 22, 2021

aminya reopened this Jul 22, 2021

Build, bootstrap, CI automation moved this from Done to In progress Jul 22, 2021

jeff-hykin added 2 commits August 29, 2021 10:25

Merge branch 'master' of https://github.com/atom-community/atom into …

7943e50

…jeff-master

-

fef50f8

jeff-hykin closed this Sep 3, 2021

Build, bootstrap, CI automation moved this from In progress to Done Sep 3, 2021

		@@ -0,0 +1 @@
		(vswhere -products * -find */Hostx86/x86/ \| select-string '(\\MSVC\\)(.*)(\\bin)').matches.groups[2].value

WIP: Add version-tracker #134

WIP: Add version-tracker #134

Conversation

jeff-hykin commented Sep 5, 2020 • edited

aminya commented Sep 5, 2020 • edited

aminya commented Sep 5, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aminya Sep 5, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeff-hykin commented Sep 5, 2020

DeeDeeG commented Sep 7, 2020 • edited

DeeDeeG Sep 7, 2020 • edited

Choose a reason for hiding this comment

jeff-hykin commented Sep 8, 2020 • edited

jeff-hykin commented Sep 8, 2020 • edited

DeeDeeG commented Sep 10, 2020 • edited

jeff-hykin commented Sep 12, 2020 • edited

aminya commented Sep 12, 2020 • edited

aminya commented Sep 16, 2020

jeff-hykin commented Sep 17, 2020 • edited

DeeDeeG commented Sep 17, 2020 • edited

aminya commented Sep 18, 2020 • edited

jeff-hykin commented Sep 18, 2020 • edited

aminya commented Sep 18, 2020 • edited

aminya commented Sep 18, 2020

jeff-hykin commented Sep 19, 2020 • edited

aminya commented Oct 2, 2020 • edited

jeff-hykin commented Oct 14, 2020

jeff-hykin commented Oct 14, 2020

aminya commented Oct 16, 2020

Choose a reason for hiding this comment

aminya Oct 19, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aminya Oct 20, 2020 • edited

Choose a reason for hiding this comment

jeff-hykin Oct 20, 2020 • edited

Choose a reason for hiding this comment

jeff-hykin Oct 20, 2020 • edited

Choose a reason for hiding this comment

jeff-hykin Oct 20, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeff-hykin Oct 20, 2020 • edited

Choose a reason for hiding this comment

jeff-hykin commented Sep 5, 2020 •

edited

aminya commented Sep 5, 2020 •

edited

aminya commented Sep 5, 2020 •

edited

aminya Sep 5, 2020 •

edited

DeeDeeG commented Sep 7, 2020 •

edited

DeeDeeG Sep 7, 2020 •

edited

jeff-hykin commented Sep 8, 2020 •

edited

jeff-hykin commented Sep 8, 2020 •

edited

DeeDeeG commented Sep 10, 2020 •

edited

jeff-hykin commented Sep 12, 2020 •

edited

aminya commented Sep 12, 2020 •

edited

jeff-hykin commented Sep 17, 2020 •

edited

DeeDeeG commented Sep 17, 2020 •

edited

aminya commented Sep 18, 2020 •

edited

jeff-hykin commented Sep 18, 2020 •

edited

aminya commented Sep 18, 2020 •

edited

jeff-hykin commented Sep 19, 2020 •

edited

aminya commented Oct 2, 2020 •

edited

aminya Oct 19, 2020 •

edited

aminya Oct 20, 2020 •

edited

jeff-hykin Oct 20, 2020 •

edited

jeff-hykin Oct 20, 2020 •

edited

jeff-hykin Oct 20, 2020 •

edited

jeff-hykin Oct 20, 2020 •

edited