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

Fix issue with multiple versions #13

Closed
wants to merge 14 commits into from

Conversation

nikolai-laevskii
Copy link

@nikolai-laevskii nikolai-laevskii commented May 12, 2023

Description

  • Updated installers
  • Refactored DotnetCoreInstaller
  • Added LTS runtime installation before requested dotnet installation (see Detailed solution description)
  • Added E2E tests that check for the problem described in the issue

Detailed solution description

When dotnet is installed after usage of some other version, install script fails to override dotnet.exe as it appears to be used by a process. Workaround is to pass --skip-unversioned-files flag to the install script and avoid overriding this file altogether. However, to ensure better compatibility and to avoid vulnerability issues, LTS runtime is now installed first, providing up-to-date unversioned files (such as CLI) for further usage.

Suggested changes

  1. We might consider running the whole solution only on Windows, as this is the only system where problem appears. This change will take minutes to implement, so it's just a matter of preference and how team decides
  2. 30 additional checks in E2E tests might be excessive, I left them here just for demonstration. I am planning to reduce their number to 3 after the review. So let me know if you want to leave some

Failed test

It doesn't always fetch install script for some reason, so it can't consistently check if it's up to date. Nothing critical.

Related issue:
actions#387

Check list:

  • Mark if documentation changes are required.
  • Mark if tests were added or updated to cover the changes.

Refactoring description

Refactoring was conducted only for installer.ts module. The most significant changes are in DotnetCoreInstaller class:

  1. Logic related to the install dir was separated and moved to simple static DotnetInstallDir class
  2. Logic related to the install script was moved to DotnetInstallScript class, which now allows to invoke install-script multiple times and conditionally apply script arguments.

Example of DotnetInstallScript class usage (just an example code, not how it is actually set here)

// Conditionally invoke install script
if (IS_WINDOWS) {
    // initialize DotnetInstallScriptClass
    const installScript: Promise<exec.ExecOutput> = await new DotnetInstallScript()
          // arguments can be inserted via this method
          .useArguments('-Runtime', 'dotnet')
          // version can be set via this method, it's just a syntactic sugar over useArguments that will insert arguments based on version provided
          .useVersion({ type: '-Channel', value: '6.0', qualityFlag: false })
          // useVersion and useArguments methods are chainable
          // script then can be then executed with execute method
          .execute();

    if (installScript.exitCode) { /* handle error */ }
}

const {exitCode, stderr} = await new DotnetInstallScript()
    // arguments do not convert automatically due the ambiguity in casing between some (such as -JSonFile and --jsonfile)
    .useArguments(IS_WINDOWS ? '-Version' : '--version', '6.0.100')
    .execute()

The script preparation is conducted upon calling the constructor and all platform-specific arguments are inserted by default

@nikolai-laevskii nikolai-laevskii self-assigned this May 12, 2023
@nikolai-laevskii nikolai-laevskii added the bug Something isn't working label May 12, 2023
Copy link

@marko-zivic-93 marko-zivic-93 left a comment

Choose a reason for hiding this comment

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

We need more eyes on this PR. It seems ok to me, but we need to make sure :)

@@ -177,7 +177,7 @@ describe('DotnetVersionResolver tests', () => {
const dotnetVersionResolver = new installer.DotnetVersionResolver(
version
);
const versionObject = await dotnetVersionResolver.createDotNetVersion();

Choose a reason for hiding this comment

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

I personally prefer the previous name of the function because it is more readable. On the other hand if most of people think it is ok to rename it, I am fine with it, this is just nitpicking :)

Copy link
Author

Choose a reason for hiding this comment

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

Changed it just for the sake of consistency as everywhere else in the repo dotnet is spelled in lower case, we can probably change it later with separate PR, if we decide that other version of spelling is better

@nikolai-laevskii
Copy link
Author

This PR is outdated and I will close it to avoid any confusion and accident merges.
It was split into two separate PRs and the whole branch was completely reorganized to make it easier to review.
Refer to the following PRs instead:

  1. Refactor installer
  2. Sequential version install fix

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants