Skip to content

Use hardlinks for portable aliases without renaming originals - #6308

Open
Kaleb Luedtke (Trenly) wants to merge 22 commits into
microsoft:masterfrom
Trenly:HardlinkAliases
Open

Use hardlinks for portable aliases without renaming originals#6308
Kaleb Luedtke (Trenly) wants to merge 22 commits into
microsoft:masterfrom
Trenly:HardlinkAliases

Conversation

@Trenly

@Trenly Kaleb Luedtke (Trenly) commented Jun 21, 2026

Copy link
Copy Markdown
Contributor

📖 Description

This PR updates portable alias handling to preserve original executable filenames and use hardlink aliases instead of renaming files.

What changed

New PortableFileType::Hardlink support (src/AppInstallerCommonCore/Public/winget/PortableFileEntry.h):

  • Added Hardlink enum value to PortableFileType
  • Added PortableFileEntry::CreateHardlinkEntry(...) factory method

New CreateHardlink filesystem helper (src/AppInstallerSharedLib/Public/winget/Filesystem.h, src/AppInstallerSharedLib/Filesystem.cpp):

  • CreateHardlink(target, link) wraps std::filesystem::create_hard_link with error handling; returns false (instead of throwing) when hardlinks are unsupported

Updated portable flow (src/AppInstallerCLICore/Workflows/PortableFlow.cpp):

  • Original executable filenames are no longer changed; the file is placed under its original name in the install directory
  • Alias names (--rename, Commands, PortableCommandAlias) are satisfied by creating a hardlink in the install directory next to the original executable
  • Symlinks in the Links directory continue to point to the original executable path
  • For archive portables, hardlinks for nested executables are placed alongside the original extracted file; a fileHashes map avoids re-reading files from disk to obtain the SHA256

Updated portable installer (src/AppInstallerCLICore/PortableInstaller.cpp):

  • Verify, install, and remove logic now handles PortableFileType::Hardlink entries (fallback to file copy if hardlinks are not supported by the volume)
  • RemoveFromPathVariable gains an onlyIfEmpty parameter to correctly handle partial-uninstall scenarios where the Links directory still contains symlinks from other packages
  • Symlink-creation logic correctly transitions the InstallDirectoryAddedToPath ARP state: adds install dir to PATH on failure, removes it again when a subsequent symlink succeeds on upgrade
  • ARP state reconstruction (InitializeExpectedEntries) now detects and tracks hardlink aliases so they are removed during uninstall even when state is rebuilt from ARP values

New E2E test manifests (src/AppInstallerCLIE2ETests/TestData/Manifests/):

  • TestZipInstaller_Portable_PathFallbackToSymlink (v1, v2) — package that switches from PATH-based install to symlink on upgrade
  • TestZipInstaller_Portable_SymlinkToPathFallback (v1, v2) — package that switches from symlink to PATH-based install on upgrade

Updated E2E tests:

  • InstallCommand.cs: 3 new tests verifying hardlink creation for --rename, Commands, and archive PortableCommandAlias
  • UninstallCommand.cs: 3 new tests verifying hardlink cleanup, correct retention/removal of the Links PATH entry when multiple packages share it, and PATH cleanup for archive portables with ArchiveBinariesDependentOnPath
  • UpgradeCommand.cs: 2 new tests verifying PATH/symlink state is correctly re-evaluated when a package switches between symlink and PATH-based aliasing across versions
  • TestCommon.cs: minor helper additions
  • Removed unused testResult variable in existing uninstall test

Updated release notes (doc/ReleaseNotes.md)

User-visible behavior

  • Portable originals are no longer renamed; the original executable exists alongside its alias in the install directory.
  • Alias names are provided as hardlinks, so commands work in non-symlinked (PATH-based) scenarios.
  • When symlink creation fails, the install directory is added to PATH as a fallback; if a subsequent upgrade can create a symlink, the install directory is removed from PATH automatically.
  • The Links directory is only removed from PATH when it becomes empty (i.e., no other symlinked packages remain).

🔗 References

🔍 Validation

  • Added E2E coverage for: hardlink install/uninstall (single exe and archive), Links PATH retention/removal across multiple packages, and PATH↔symlink state transitions on upgrade.
  • Local manual validation completed.

✅ Checklist

📋 Issue Type

  • Bug fix
  • Feature
  • Task

Created with assistance from GitHub Copilot.

@github-actions

This comment has been minimized.

Kaleb Luedtke (Trenly) and others added 2 commits June 20, 2026 21:36
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@Trenly
Kaleb Luedtke (Trenly) marked this pull request as ready for review June 21, 2026 02:41
@Trenly
Kaleb Luedtke (Trenly) requested a review from a team as a code owner June 21, 2026 02:41
@Trenly

Copy link
Copy Markdown
Contributor Author

This implementation is based largely in part on #6270. I believe the approach of creating the hardlinks next to the original executables instead of as a fallback within the symlink flow has the following benefits:

  • The hardlink as an alias will exist regardless of if the package is added to path or symlinked. This has the benefit of not depending on the symlink flow at all, reducing potential points of failure.
  • The hardlink is placed in the same directory as the executable - avoiding any concerns about same-volume restrictions
  • In non-symlinked installations, the package directory is added to path, the hardlink ensures the alias will be available
  • When ArchiveBinariesDependOnPath is set, the symlink creation is never attempted - this approach ensures the alias is created as the logic does not depend on the symlink flow
  • Hardlink creation is directly tracked as part of the install process, ensuring proper cleanup during uninstall

@rich-purnell

Rich Purnell (rich-purnell) commented Jun 22, 2026

Copy link
Copy Markdown

@Trenly During testing, I identified the following issues. For brevity, I'll use PM to indicate running with administrator privileges or Developer Mode enabled, and NPM to indicate running without administrator privileges and with Developer Mode disabled.

  1. When installing and uninstalling GodotEngine under NPM, the installation directory is not removed from PATH, because when attempting to delete it, the directory is not empty (the *.db files have not been deleted yet).

Test commands:

# NPM
wingetdev install -e --id GodotEngine.GodotEngine
wingetdev uninstall -e --id GodotEngine.GodotEngine
# Open a new terminal window and run it.
Write-Output $env:Path | Select-String GodotEngine
  1. When installing GodotEngine under NPM and then updating under PM, no symlink is created because InstallDirectoryAddedToPath was set to true during the initial installation and persisted.

Test commands:

# NPM
wingetdev install -e --id GodotEngine.GodotEngine -v 4.6.3
# PM
wingetdev update -e --id GodotEngine.GodotEngine
  1. When installing GodotEngine under PM and then updating under NPM, the Links directory is not cleaned from PATH (if the Links directory is empty) because InstallDirectoryAddedToPath is true during the update, skipping RemoveFromPathVariable(GetPortableLinksLocation(GetScope()));.

Test commands:

# PM
wingetdev install -e --id GodotEngine.GodotEngine -v 4.6.3
# NPM
wingetdev update -e --id GodotEngine.GodotEngine

@Trenly

Copy link
Copy Markdown
Contributor Author

Thanks for the feedback Rich Purnell (@rich-purnell) - Will look into those scenarios!!

@rich-purnell

Copy link
Copy Markdown

Regardless of which approach is chosen, using InstallDirectoryAddedToPath to gate both AddToPathVariable() and RemoveFromPathVariable() doesn't seem quite reasonable, and persisting it appears unnecessary as well.

@JohnMcPMS JohnMcPMS left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I prefer this fix over creating hardlinks in the link directory due to complications discussed in that PR.

Comment thread src/AppInstallerSharedLib/Filesystem.cpp Outdated
Comment thread src/AppInstallerCommonCore/Public/winget/PortableFileEntry.h Outdated
Comment thread src/AppInstallerCLICore/Workflows/PortableFlow.cpp Outdated
Comment thread src/AppInstallerCLICore/Workflows/PortableFlow.cpp Outdated
Comment thread src/AppInstallerCLICore/Workflows/PortableFlow.cpp Outdated
Comment thread src/AppInstallerCLICore/PortableInstaller.cpp Outdated
@JohnMcPMS

Copy link
Copy Markdown
Member

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@Trenly

Copy link
Copy Markdown
Contributor Author

JohnMcPMS - Resolved the merge conflicts

@denelon

Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

@Trenly

Copy link
Copy Markdown
Contributor Author

@JohnMcPMS - I want to confirm my assumption, since I'm not seeing these two failures when I run the tests:

image

My question is - It seems there is no set up / teardown for these tests that ensures a clean state between the tests. I assume this means that the links directory is remaining on path for these two tests which is causing the failure. Would it be correct to add a [SetUp] section to uninstall any portable packages from previous tests?

@JohnMcPMS JohnMcPMS left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not a huge fan of rewriting the test assertions just to do it.

Comment thread src/AppInstallerCLICore/Workflows/PortableFlow.cpp Outdated
@JohnMcPMS

Copy link
Copy Markdown
Member

My question is - It seems there is no set up / teardown for these tests that ensures a clean state between the tests. I assume this means that the links directory is remaining on path for these two tests which is causing the failure. Would it be correct to add a [SetUp] section to uninstall any portable packages from previous tests?

The tests are failing with a SQL error that suggests to me a missing change database. You can diagnose issues from the E2E test in the pipeline by:

  1. Download the logs files from the artifacts for the flavor
  2. Look at the attachments for the failing test in pipeline
  3. The stdout attachment will have correlation GUIDs for every command, search the log files for that GUID
  4. Look at what happened in the CLI logs for more indication on what went wrong

@Trenly

Copy link
Copy Markdown
Contributor Author

My question is - It seems there is no set up / teardown for these tests that ensures a clean state between the tests. I assume this means that the links directory is remaining on path for these two tests which is causing the failure. Would it be correct to add a [SetUp] section to uninstall any portable packages from previous tests?

The tests are failing with a SQL error that suggests to me a missing change database. You can diagnose issues from the E2E test in the pipeline by:

  1. Download the logs files from the artifacts for the flavor
  2. Look at the attachments for the failing test in pipeline
  3. The stdout attachment will have correlation GUIDs for every command, search the log files for that GUID
  4. Look at what happened in the CLI logs for more indication on what went wrong

Thank you for the help in finding this; That cleared up why these are failing - the Ids are too long and are running into path length limits.

Kaleb Luedtke (Trenly) and others added 4 commits August 5, 2026 21:09
Instead of silently falling back to an empty SHA256 string when a
hardlink target is not found in the fileHashes map, throw
APPINSTALLER_CLI_ERROR_PORTABLE_INSTALL_FAILED with a descriptive
message. Any file being hardlinked should always be present in the map.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

4 participants