Skip to content

Conversation

@beyondkmp
Copy link
Contributor

@beyondkmp beyondkmp commented Jun 4, 2025

fix #9092

The app output of electron-builder is placed in a temporary directory, so we can directly copy all files into lib\net45 without needing to perform filtering.

@changeset-bot
Copy link

changeset-bot bot commented Jun 4, 2025

⚠️ No Changeset found

Latest commit: cba48ef

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@mmaietta
Copy link
Collaborator

mmaietta commented Jun 4, 2025

I don't think this is a necessary change. I'd prefer explicit file includes as opposed to a glob + exclude filter. Is there a performance benefit to this PR?

@beyondkmp
Copy link
Contributor Author

The old version used to copy all files into \lib\net45, so this approach doesn't seem to be an issue.

@mmaietta
Copy link
Collaborator

mmaietta commented Jun 6, 2025

The old version used to copy all files into \lib\net45, so this approach doesn't seem to be an issue.

I don't see a reason for this change from the current functionality being provided - this simplification doesn't seem needed. Allowlists are always my more preferred approach when it comes to production assets, not blocklists using exclude="**\.git and other exclusions as we can't guarantee if other files might accidentally be included by the end-developer's app configuration/setup.

If we're needing to handle extraFiles in being included, then can we explicitly pipe those into the inclusion array?

@beyondkmp
Copy link
Contributor Author

@mmaietta How about we make this template file customizable? If users need to include more files, they could specify their own template. If none is specified, the current default template would be used.

@mmaietta
Copy link
Collaborator

Hmmm, that's a great proposal to handle very advanced use cases, not sure if the average dev will understand how to go that deep into nuspec.

Is there a way we could just loop through a property on squirrel for customizing the accepted/to-be-copied files?
Similar how it already does for:

    <file src="<%- exe %>" target="lib\net45\<%- exe %>" />
    <% additionalFiles.forEach(function(f) { %>
    <file src="<%- f.src %>" target="<%- f.target %>" />
    <% }); %>

@github-actions
Copy link
Contributor

This PR is stale because it has been open for 30 days with no activity. Remove stale label or comment, or this will be closed in 30 days.

@github-actions github-actions bot added the Stale label Jul 21, 2025
@mmaietta
Copy link
Collaborator

mmaietta commented Aug 4, 2025

@beyondkmp friendly ping on #9140 (comment)

Let me know what the best next steps are, but currently, I'm not a fan of having a globstar "all" regex

@github-actions github-actions bot removed the Stale label Aug 5, 2025
@beyondkmp
Copy link
Contributor Author

No better solution has been identified yet. The previous approach used by the old Squirrel.Windows version was to package all contents from the app directory into lib\net45 without any filtering. Therefore, the current idea is to simply include the entire app directory without any filtering.

…s dynamically

- Modified `createNuspecTemplateWithProjectUrl` to accept `additionalFiles` as a parameter.
- Replaced the static additional files loop in the nuspec template with dynamic content based on the provided files.
- Added a new method `getAdditionalFiles` to filter and prepare additional files for inclusion in the nuspec template.
beyondkmp and others added 4 commits August 11, 2025 10:14
- Removed the `exeName` parameter from the `getAdditionalFiles` method call.
- Updated test fixtures to include `index.html` as an extra file for Squirrel Windows packaging.
- Added `packageManager` field to the test app's `package.json` for consistency.
const additionalFilesContent = additionalFiles.map(f => ` <file src="${f.src}" target="${f.target}" />`).join("\n")
templateContent = templateContent.replace('<% file src="additionalFiles.src" target="additionalFiles.target" / %>', additionalFilesContent)
} else {
templateContent = templateContent.replace('<% file src="additionalFiles.src" target="additionalFiles.target" / %>', "")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I like this approach!

Can you create a const for this? (similar to searchString on line 170)

const additionalFilesReplaceKey = '<% file src="additionalFiles.src" target="additionalFiles.target" / %>'

"lib/net45/d3dcompiler_47.dll",
"lib/net45/ffmpeg.dll",
"lib/net45/icudtl.dat",
"lib/net45/index.html",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this expected behavior? If we're testing just extraFiles, then I'd rather it not be the index.html since it may get misconstrued as part of the normal bundle when comparing manually comparing snapshot diffs.

Maybe create a temp file in the unit test?

@github-actions
Copy link
Contributor

github-actions bot commented Oct 2, 2025

This PR is stale because it has been open for 30 days with no activity. Remove stale label or comment, or this will be closed in 30 days.

@github-actions github-actions bot added the Stale label Oct 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

.exe files not included in package after upgrading from 25.1.8 to 26.0.12

2 participants