Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow us to build vscode against system's electron #56686

Closed
FFY00 opened this issue Aug 17, 2018 · 24 comments
Closed

Allow us to build vscode against system's electron #56686

FFY00 opened this issue Aug 17, 2018 · 24 comments
Assignees
Labels
feature-request Request for new features or functionality *out-of-scope Posted issue is not in scope of VS Code workbench-electron Electron-VS Code issues

Comments

@FFY00
Copy link

FFY00 commented Aug 17, 2018

Hello, Arch Linux TU here.

I want to move vscode to the [community] repo but I haven't found a way to build the project against system's electron as opposed to bundling electron together with it.

Can we merge something to the upstream that would make this easier?
Also, getting feedback/help from the devs seems easier than trying to make it work all by myself. If anyone has a solution for this, please let me know.

Thanks,
Filipe

@bpasero bpasero added feature-request Request for new features or functionality workbench-electron Electron-VS Code issues labels Aug 20, 2018
@joaomoreno
Copy link
Member

I want to move vscode to the [community] repo but I haven't found a way to build the project against system's electron as opposed to bundling electron together with it.

Can you elaborate?

@joaomoreno joaomoreno added the info-needed Issue requires more information from poster label Aug 21, 2018
@eli-schwartz
Copy link

eli-schwartz commented Aug 21, 2018

We provide a global electron installation: https://www.archlinux.org/packages/community/x86_64/electron/

Distribution packages which use electron are expected to use this rather than vendor private copies, for all the reasons Linux distributions usually like to use system libraries for things. Our other "official" Arch Linux packages will usually use wrapper shell scripts that run:

exec electron /path/to/app

While only packaging the js code usually stored in "resources/app" in an electron-builder or electron-packager prebuilt archive.

But vscode seems to be far more complex to do this debundling properly.

@joaomoreno joaomoreno removed the info-needed Issue requires more information from poster label Aug 22, 2018
@joaomoreno
Copy link
Member

I see. This will very likely not be officially supported by us. Even if you get it working, you wouldn't be able to call it VS Code, because #60 (comment)

Can we merge something to the upstream that would make this easier?

Without getting to actual coding, what changes would you need?

@eli-schwartz
Copy link

you wouldn't be able to call it VS Code

Well, yeah, the package that would potentially get moved to [community], is https://aur.archlinux.org/packages/code

"Microsoft Code -- The Open Source build of Visual Studio Code (vscode)"

Without getting to actual coding, what changes would you need?

I don't know the exact details (I don't use this myself), but see for example #55934 (comment)

If vscode executed correctly using /usr/bin/electron, that would be the first step. The second step would be cleaning up the build process.

@Tyriar
Copy link
Member

Tyriar commented Aug 22, 2018

For #55934, the issue looks to be some issue with Electron 2 and Arch, you can't just workaround this by plugging Electron 3 in if that's what you're thinking due to this:

NODE_MODULE_VERSION 57. This version of Node.js requires
NODE_MODULE_VERSION 64. Please try re-compiling or re-installing

Electron wasn't built to be a shared library, it all falls apart when you start to use native node modules (at least until N-API is widespread). To get this to work Arch needs to target the specific version of Electron that VS Code targets.

Is the main purpose of this to share the Electron binary (with the correct version), because that seems like a lot of work.

@microsoft microsoft deleted a comment Aug 24, 2018
@eli-schwartz
Copy link

Is the main purpose of this to share the Electron binary (with the correct version), because that seems like a lot of work.

From the perspective of a Linux distro, it's less work than ensuring that various static libraries are kept up to date with security fixes. It's less work than making sure electron is kept up to date (by updating the version ourselves).
It has the additional benefit of being significantly smaller in download size for users who may potentially have limited bandwidth.

This is essentially the fundamental story of "why distributions use system anything". It's been argued for and against, by many people through the years, so there's not much more to say, really.

@johnramsden
Copy link

Isn't electron meant to be used in this way anyway? You have it on your system once, and everything that wants to use it shares it, rather than having it multiple times.

Clearly this isn't the way it's used in practice, as many projects bundle a copy of electron. It seems like allowing code to use the system electron would be a step in the right direction.

@Tyriar
Copy link
Member

Tyriar commented Aug 29, 2018

@johnramsden no, it was never meant to work that way. AFAIK Arch is going into unexplored territory by attempting to do it, which is why officially supporting it probably isn't an option (yet) as @joaomoreno says.

There are certainly a lot of advantages to doing it the way you're suggesting and we've even spoken about it with the Electron team before.

@eli-schwartz
Copy link

eli-schwartz commented Aug 29, 2018

no, it was never meant to work that way.

This "never meant to work that way" thing explains why electron is extremely specifically designed to be a single binary artifact that is unpacked and renamed in order to rebrand it as whatever packaged electron app you like... right?
This also explains why electron has a builtin feature to load an app by filepath in addition to using resources/app... right?

AFAIK Arch is going into unexplored territory by attempting to do it,

Untrue, Gentoo at least also does this: https://bugs.gentoo.org/579116

There's also an unofficial Fedora repository based on the work of Arch's package: https://copr.fedorainfracloud.org/coprs/mosquito/atom/

🤷‍♂️ People want to be able to install Atom on their distro. It's somewhat more popular anyway than the many tons of other based-on-electron software.

@Tyriar
Copy link
Member

Tyriar commented Aug 29, 2018

@eli-schwartz to my knowledge that was never the intent, I'm agreeing that it's a good direction for Electron to go though.

@joaomoreno
Copy link
Member

This won't likely happen from our side. Sorry guys.

@joaomoreno joaomoreno added the *out-of-scope Posted issue is not in scope of VS Code label Sep 19, 2018
@vscodebot
Copy link

vscodebot bot commented Sep 19, 2018

This iteration we focus on issue grooming. This issue is being closed to keep the number of issues in our inbox on a manageable level, we are closing issues that are not going to be addressed in the foreseeable future: We look at the number of votes the issue has received and the number of duplicate issues filed. More details here. If you disagree and feel that this issue is crucial: We are happy to listen and to reconsider.

If you wonder what we are up to, please see our roadmap and issue reporting guidelines.

Thanks for your understanding and happy coding!

@vscodebot vscodebot bot closed this as completed Sep 19, 2018
@eli-schwartz
Copy link

We currently have this building using the system electron package. See https://git.archlinux.org/svntogit/community.git/tree/trunk?h=packages/code

Would you be interested in incorporating any of these changes or making them easier to integrate?

@FFY00
Copy link
Author

FFY00 commented Sep 20, 2018

They clearly stated that they aren't in #57711.

@joaomoreno
Copy link
Member

@eli-schwartz Note that it's mostly a legal issue, and not really a technical one. Can you explicitly point out the changes to me?

@eli-schwartz
Copy link

eli-schwartz commented Sep 20, 2018

  • We use a custom loader script code.js
  • The executable in /usr/bin/ is a wrapper script calling electron /usr/lib/code/out/cli.js /usr/lib/code/code.js
  • We sed out .yarnrc with the electron version from the system installation
  • We install VSCode-linux-x64/resources/app/ as /usr/lib/code

@eli-schwartz
Copy link

it's mostly a legal issue, and not really a technical one

Do you mean to say you just want to know, in order to make sure it's not infringing the Microsoft copyright? I'm really unsure what that means, since after all the source code repository we use is MIT-licensed, which gives us a tremendous amount of latitude.

Do you mean you want to know if it's licensed in a way that allows merging? Then you should ask @FFY00 to agree to license it under the same MIT license...

@joaomoreno
Copy link
Member

  • Would this custom loader script be useful for other scenarios? It seems to be very specific to the situation of launching vanilla electron against Code's resources.
  • I don't think the .yarnrc changes would make sense to go upstream.
  • The install location is already defined for the deb and rpm packages, I don't think we want to change it now.

Do you mean to say you just want to know, in order to make sure it's not infringing the Microsoft copyright? I'm really unsure what that means, since after all the source code repository we use is MIT-licensed, which gives us a tremendous amount of latitude.

Yeah I meant it that way. Everything in this repo is MIT yes, but not everything in the product is. Namely, the changes in product_json.patch are not. I will reach out to someone who knows this better and can provide guidance.

@eli-schwartz
Copy link

Hmm, uh.

Well, there is #31168 and microsoft/vscode-wiki#30, plus the ignored microsoft/vscode-wiki#34
Which seem to be sort of conflicting.

I don't see anywhere where it is spelled out, whether it is intended that open-source builds be allowed to access the marketplace -- or whether that is simply not enabled by default.

@FFY00
Copy link
Author

FFY00 commented Sep 20, 2018

  • Would this custom loader script be useful for other scenarios? It seems to be very specific to the situation of launching vanilla electron against Code's resources.

One place I see this being extremely helpful would be when developing/making changes to vscode. You wouldn't need to recompile anything, just make the changes and launch it. You can invoke electron directly in the code. If anything you would need to regenerate the modified typescript files. IMO this would help everyone, by increasing your productivity and by allowing us to package vscode properly.

  • I don't think the .yarnrc changes would make sense to go upstream.

Me neither. Electron should be able to run either way unless you make some changes that are not supported by some specific versions. We only patch it to make sure that isn't the case.

  • The install location is already defined for the deb and rpm packages, I don't think we want to change it now.

Doesn't need to be static, you can resolve it like you do in the bash scripts.

@microsoft microsoft deleted a comment Sep 20, 2018
@FFY00
Copy link
Author

FFY00 commented Sep 28, 2018

@joaomoreno did you have a look at the point presented? I think it would be very beneficial for you too.

@FFY00
Copy link
Author

FFY00 commented Oct 9, 2018

@joaomoreno do you have any update on this?

@joaomoreno
Copy link
Member

@FFY00 Yes, there is no commitment from our side, as mentioned before. We already have a very good development flow for VS Code in which we don't need to recompile anything. Thanks for understanding.

As for the licensing guidance, that's still ongoing.

@FFY00
Copy link
Author

FFY00 commented Oct 9, 2018

No need to change anything related to the license. I am not packaging this as a microsoft product.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request Request for new features or functionality *out-of-scope Posted issue is not in scope of VS Code workbench-electron Electron-VS Code issues
Projects
None yet
Development

No branches or pull requests

6 participants