-
-
Notifications
You must be signed in to change notification settings - Fork 740
electron-builder: Provide correct output path #942
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR moves PR comment creation to a separate workflow using pull_request_target trigger. The change updates the electron-builder command configuration to explicitly specify the output directory using the ElectronPublishUrlFullPath property.
Key Changes:
- Modified the electron-builder command to include an explicit output directory configuration parameter
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
0dd97c4 to
9bb6454
Compare
|
@FlorianRappl - Can we merge this? It broke my build (after switching to our new packages from here)... |
|
|
||
| <PropertyGroup> | ||
| <_NpxCmd>npx electron-builder --config=./$(ElectronBuilderJson) --$(ElectronPlatform) --$(ElectronArch) -c.electronVersion=$(ElectronVersion) $(ElectronPaParams)</_NpxCmd> | ||
| <_NpxCmd>npx electron-builder --config=./$(ElectronBuilderJson) --$(ElectronPlatform) --$(ElectronArch) -c.electronVersion=$(ElectronVersion) -c.directories.output "$(ElectronPublishUrlFullPath)" $(ElectronPaParams)</_NpxCmd> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I removed that (need to verify). For me this resulted in a recursive loop as the output was in the same directory as the signing. That was really bad.
Since the output is anyway placed in the CWD (which is the publish directory) I don't see the direct reason for this. For me, the publish only worked when this was removed. Maybe it was an edge case, but I used a new project with a default publish config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The point is that the output should go in the directory that you specify in the publish profile.
When I publish the WebApp with the included profiles, all is working fine. In which configuration did it fail for you?
(electron-builder)?
When the output is not specified, it writes the output into a "dist" folder instead of the publish output folder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you ran publish without profile and without specifying a publish output folder?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No I published with a profile. And it published to the given directory, however, it got stuck in the process (as described) and the signing tool started go berserk (only seen with verbose flags provided, otherwise all of that is hidden).
The error is really nasty and as written you don't need that flag. The PublishUrl we use to specify the CWD, so I don't see why we should double it. For me it only caused havoc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even if I would not have published without a profile I don't why we should specify this. The error is really nasty as it does not only stay hidden (and fails to complete the publish process), it also eats up essentially all the disk space if not stopped in time.
Again, as the publish is done in the CWD and the electron-builder just works as designed I don't see why we should be explicit here. Have you maybe skipped signing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I think it's when
`<ElectronNetDevMode>true</ElectronNetDevMode>`
is enabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have seen this with the actual packages, but I guess / fear that with the dev mode it might also be true. TBH I think the actual problem is within the electron-builder. Even setting some directories as ignored did not help. It should never act like this. But we can't patch / change the electron-builder.
No, this breaks my publish process (recursive creation of directories & files). The error here is really nasty. We first need to check if this is still the case. Can you specify how it breaks your build? The electron-builder runs in the CWD specified as the publish directory and for me this is all that's needed. It will work with the defaults just fine. What exactly is breaking if used with the defaults? |
That's a bit tricky. There is a fundemental different between MBuild publish for AspNetCore and a normal projects. That's why we're seeing different things. |
|
It's also not the same path. One is ElectronPublishDirFullPath and the other is ElectronPublishUrlFullPath which are IIRC same in one case and different in the other.,
Using the actual packages or with ElectronNetDevMode set true? |
|
With the current package, when publishing from VS (console app), it goes into:
instead of here:
|
Actual packages.
That's true. My scenario was an ASP.NET Core project. Presumably, we maybe should add a flag / only use it if the directories won't match / overlap? |
|
Let me check what's happening. It has been working fine for a long time. I've switched to the console model just very recently. I see what's happening. Nested folder ad infinity - explorer and VS are crashing... |
Yes, that is what occurred on my system. Did I mention that this was "nasty"? Windows actually had problems deleting the folder as the depth was too high... |
Right. Even I don't know whether I've ever seen cmd itself crashing... But PowerShell did it. One thought: Maybe it's Windows-specific. I've tested all the publishing before submitting the first PR, but maybe not Windows as target. What is this signing anyway? What does it want to sign and how - without a cert? |
|
I am 100% sure its Windows specific. My guess is that electron-builder has a problem with the backslashes and compares unix formatted paths with the Windows paths. In Node.js both are equivalent, but if you string compare... well - then two equal paths are no longer equal. |
9bb6454 to
4aaf736
Compare
|
I've updated this PR accordingly. |
4aaf736 to
d05191d
Compare
Update
|
|
Originally that change originated from #900 - I just tested the latest release (preview version) and the issue is back with your changes. Can you have a look? |
|
|
Yup, will check |
|
Alright, in that project (https://github.com/mu88/BlazorFotoManager), the publish config in .pubxml is wrong. Should just copy ours. Also, the electron-builder.json had a lot of things it should not have. I think we need to better document the migration. I thought it would be fine to say they need to copy the "build" subtree from package.json, but we also need to explain what needs to be removed. Here's a fixed version of the foto manager which publishes fine with our latest packages: https://github.com/softworkz/BlazorFotoManager/tree/fix_electronnet |
|
To be more specific: The publish profile in this project was wither copied from our console test app or from another non-WebSDK project. These are using The logic in the LateImport.targets file is admittedly hard to follow. A prerequisite for that is to know which targets are run for one or the other publishing method and which run in both cases. And the order of execution. Then it's pretty easy to understand :-) |
|
Yeah I assumed already its outdated / wrong, but I think we should handle this case better. For instance, if a specific variable is not set we should actually error out and print a good message. Otherwise, more people might fall into that trap (e.g., during their migration). Example: Should we check if |
Yes, absolutely but only in the case of WebSDK publishing. Somewhere there's a variable being set whether it's WebSDK or not. That would be the condition. Migration issues would be about electron-builder.json. There we need to check for all folder-related values to make sure none are specified. |
Especially everything with relative parent paths like So the pattern is always like:
|
|
I attempted publishing on my mac and I saw similar behavior so this is not windows specific. (I'm using 0.2) It didn't create an infinite number of sub directories... it's stops eventually when the app bundle file has zero bytes. The top level folder where the first bundle lives is about 4 times as big as it should be. If you descend into the next directory the bundle is slightly smaller and so on and on and on. The interesting thing is the top level app bundle runs as you would expect. if i just build a release version and run electron-builder manually from the command line it works the way it's supposed to. |
|
@markatosi - please check out my commit here: mu88/BlazorFotoManager@main...softworkz:BlazorFotoManager:fix_electronnet to see how I fixed that other project. |
|
Thank you. I'll check it out in a bit, time for lunch :-) I'll let you know if it resolves my issue soon. No luck so far... I'm going to create a mostly empty blazor app and try and get this working instead of using a copy of my real app to make sure there isn't something else going on. |
|
The bare bones demo app published perfectly. On my production app i removed the bin and obj directories. That did nothing. I need a beer. It's always the most obvious thing that i ignore |

This had gotten lost somehow in my original PR.