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

(virtualbox) Add missing dependency for 7.0 stream #2041

Merged

Conversation

brogers5
Copy link
Contributor

@brogers5 brogers5 commented Oct 29, 2022

Description

This changeset adds a 7.0 stream-specific dependency on vcredist140 v14.20.27508.1 or later to the virtualbox package.

Motivation and Context

Fixes #2005.

The main challenge behind implementing this was the fact that virtualbox also supports a separate stream for the 6.1 maintenance branch. Unlike the 7.0 branch, this dependency does not apply for the 6.1 branch. Ideally, we'd like to avoid having 6.1 take on an unnecessary dependency.

In looking around the repository, I found some update helper functions for dependency management in the 1password package, which I felt would be very helpful here. I opted to extract these to generic AU extension functions to enable use of a common implementation for both virtualbox and any current/future packages that may encounter a similar problem down the road.

How Has this Been Tested?

virtualbox Update Test

Temporarily reverted virtualbox.json to the pre-6.1.40 state.

Executed the modified update.ps1 with NoCheckChocoVersion.

Inspected the resulting virtualbox packages (6.1.40 and 7.0.2) with NuGet Package Explorer.

Confirm that both packages maintain a dependency on chocolatey-core.extension v1.3.3 or later.

Confirmed the 6.1.40 package does NOT have a dependency on vcredist140.
Confirmed the 7.0.2 package DOES have a dependency on vcredist140 v14.20.27508.1 or later.

virtualBox Install Test

Copied the 7.0.2 test package to a local instance of a clean Chocolatey Testing Environment.

Installed the test package using choco install virtualbox --source="'.;https://community.chocolatey.org/api/v2/'".

Confirmed the installation succeeds.

1password4 Update Regression Test

Set $Force = $true
Set $IncludeStream = 'OPW4'
Import-Module Wormies-AU-Helpers to avoid unrecognized function errors (I assume update.ps1 currently expects test_all.ps1 or update_all.ps1 to set this up).

Execute the modified update.ps1.

Inspect the resulting 1password4 package with NuGet Package Explorer.

Confirm that 1password4 maintains a dependency on chocolatey-core.extension v1.3.3 or later.

Confirm that 1password4 does NOT have a dependency on dotnet4.7.2.

1password Update Regression Test

Note
This assumes we're using the same PowerShell session as with the last test.

Set $IncludeStream = 'OPW'

Execute the modified update.ps1.

Inspect the resulting 1password package with NuGet Package Explorer.

Confirm that 1password maintains a dependency on chocolatey-core.extension v1.3.3 or later.

Confirm that 1password also maintains a dependency on dotnet4.7.2 v4.7.2.20180712 or later.

Screenshot (if appropriate, usually isn't needed):

N/A

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Migrated package (a package has been migrated from another repository)

Checklist:

  • My code follows the code style of this repository.
  • My change requires a change to documentation (this usually means the notes in the description of a package).
  • I have updated the documentation accordingly (this usually means the notes in the description of a package).
  • I have updated the package description and it is less than 4000 characters.
  • All files are up to date with the latest Contributing Guidelines
  • The added/modified package passed install/uninstall in the chocolatey test environment.
  • The changes only affect a single package (not including meta package).

… for planned reuse in VirtualBox package, fix PSScriptAnalyzer warnings, remove unnecessary namespace declarations
…al C++ 2015-2019 Redistributable v14.20.27508.1
@AppVeyorBot
Copy link

❌ Package verification failed, please review the Appveyor Logs and the provided Artifacts before requesting a human reviewer to take a look.

@brogers5
Copy link
Contributor Author

brogers5 commented Oct 29, 2022

Verification failed because 1password4's uninstallation process is currently not silent:

Uninstall screenshot

This appears to be a regression that was introduced with 7c18a07, which causes both 1password and 1password4 to use a common silentArgs value in the uninstall script (presumably intended for 1password), which fails in 1password4's case. Reverting this commit and repacking seems to resolve the issue.

I can push a new commit implementing this if need be, although it seems we wanted to go in a different direction as previously documented with #1771. Not sure if this is within the scope of this PR to resolve.

@RedBaron2
Copy link
Contributor

@brogers5
I don't understand why the ternary was removed in that commit. If you would like to update the package with a PR to fix this issue. I'm sure it would be much appreciated.

Copy link
Member

@AdmiringWorm AdmiringWorm left a comment

Choose a reason for hiding this comment

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

As per 3.2 pull requests should in general only affect a single package (there are exceptions, but this PR is not one of them), as this package is for VirtualBox according to the title please update the PR to only make the necessary changes to the VirtualBox while dropping off the changes to 1password.

automatic/1password/update.ps1 Outdated Show resolved Hide resolved
automatic/virtualbox/update.ps1 Outdated Show resolved Hide resolved
@AppVeyorBot
Copy link

✅ Package verification completed without issues. PR is now pending human review

Copy link
Member

@AdmiringWorm AdmiringWorm left a comment

Choose a reason for hiding this comment

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

We probably need to rework the scripts at a later time, but for now it is fine.

LGTM, thank you for making these changes.

@AdmiringWorm AdmiringWorm merged commit ebed232 into chocolatey-community:master Dec 16, 2022
@AdmiringWorm
Copy link
Member

@brogers5 your changes have been merged, thanks for your contribution 👍

Your changes will be part of any new package versions that will be pushed to the Chocolatey Community Repository.

@brogers5 brogers5 deleted the virtualbox-add-dependency branch December 16, 2022 23:15
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.

(virtualbox) Missing new dependency on vcredist140 v14.20.27508.1 or later
5 participants