Skip to content

refactor: simplify nuspec file structure by consolidating file entries into a single pattern #9140

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

beyondkmp
Copy link
Collaborator

@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.

Copy link

changeset-bot bot commented Jun 4, 2025

⚠️ No Changeset found

Latest commit: 557e8c1

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
Collaborator 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
Collaborator 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 %>" />
    <% }); %>

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
Collaborator 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.
- 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.
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