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

get rid of nuget #77

Closed
wants to merge 1 commit into from
Closed

get rid of nuget #77

wants to merge 1 commit into from

Conversation

develar
Copy link
Contributor

@develar develar commented Apr 26, 2016

Speed up 4x-8x (only nuget part, Squirrel.Windows is another part of story).

Related issues: #55, electron-userland/electron-builder#351

Works in electron-builder (it is the reason why I created PR only now).

This PR blocks PR about code signing on OS X (it is fixed in my fork also).

Speed up 4x-8x
@@ -162,5 +162,5 @@ function handleSquirrelEvent() {
You can get debug messages from this package by running with the environment variable `DEBUG=electron-windows-installer:main` e.g.

```
DEBUG=electron-windows-installer:main node tasks/electron-winstaller.js
DEBUG=electron-windows-installer node tasks/electron-winstaller.js
Copy link
Contributor Author

@develar develar Apr 26, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want to debug — you need all categories in any case.

The issue — if you use different categories, you cannot get time information (debug measures time per category).

@anaisbetts
Copy link
Contributor

This almost certainly has correctness issues because you hard-code the list of extensions in the content types file. Why do we care about the speed that packaging happens again?

@develar
Copy link
Contributor Author

develar commented Apr 30, 2016

@paulcbetts Thanks for review.

because you hard-code the list of extensions in the content types file

Why it is important? Where Squirrel.Windows uses it? If it is required, I think, we should fix it and don't require?

Why do we care about the speed that packaging happens again?

Because 10 minutes (or hang) is not acceptable. electron-userland/electron-builder#351 — "in at 99% CPU usage for about ten minutes.".

And here no sense to fix nuget or Squirrel.Windows — it is very easy to write cross-platform solution to pack (I don't say that node is perfect — but .net platform is definitely not suitable here).

@anaisbetts
Copy link
Contributor

Why it is important? Where Squirrel.Windows uses it? If it is required, I think, we should fix it and don't require?

NuGet uses it, Squirrel.Windows doesn't really care

In general, I'd rather favor correctness over speed - this PR already has issues with generating correctly formed NuGet packages and I don't really want to be in the situation where a bunch of apps now report "My installer is broken and I just shipped it to 12030232x users!" We cannot ship broken updates, and determining whether this fix is correct in 100% of scenarios is not trivial. Sorry.

@anaisbetts anaisbetts closed this Apr 30, 2016
@anaisbetts
Copy link
Contributor

If someone can give me a repro of the 10-minute builds, I can look into why NuGet.exe is taking so long

@develar
Copy link
Contributor Author

develar commented May 1, 2016

@ArekSredzki Is it possible to attach your project? It seems, any project without asar and a lot of files is an example, but it will be better to have real app.

@paulcbetts Both problems — on nuget and Squirrel.windows — in the zip library, I guess (see electron-userland/electron-builder#351 (comment) log)

@develar
Copy link
Contributor Author

develar commented May 1, 2016

@paulcbetts Also, nuget is a plain zip file with a some predefined structure.

NuGet uses it, Squirrel.Windows doesn't really care

How? Why it is important? App files are using only by Electron, for nuget it can be safely be a just Content-Type: application/octet-stream.

If it is the only problem, I don't think it is even has minor priority. Because as result we don't depend on win-only tool.

Squirrel.Windows doesn't really care

We build setup.exe for Squirrel.Windows, not for nuget, only for Squirrel.Windows. It is cool that Squirrel.Windows doesn't use this bad-smell and questionable approach to define Content-Types. So, why it is a problem?

BTW, actually, there is already alternative implementation of nuget pack — http://docs.octopusdeploy.com/display/OD/Using+Octo.exe And in any case we don't try to reimplement nuget pack — we just need to provide data to Squirrel.Windows. https://github.com/jordansissel/fpm allows us just provide directory mappings. It will be an ideal solution here. But currently Squirrel.Windows requires questionable nuget file. I understand the reasons — Squirrel.Windows was developer for windows apps. But Electron app is just a bunch of files (no deps).

Also — electron-builder tests check resulting nupkg (full), so, if content is the same, I am sure that we will not get "My installer is broken and I just shipped it to 12030232x users!" Please note — again, we don't build nuget package, we build setup.exe and update files or Squirrel.Windows (nuget or delta — it is all not about nuget, but about Squirrel.Windows (and ideally, 7z compression should be used (I have exact number for mac— 28 vs 40 MB)).

@Nowaker
Copy link

Nowaker commented Aug 2, 2017

So sad to see this closed. Our Mono builds are extremely slow. It's 32 minutes and counting...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants