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

[MNG-7541] Implement powershell command #878

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

Conversation

JurrianFahner
Copy link

@JurrianFahner JurrianFahner commented Nov 19, 2022

Following this checklist to help us incorporate your
contribution quickly and easily:

  • Make sure there is a JIRA issue filed
    for the change (usually before you start working on it). Trivial changes like typos do not
    require a JIRA issue. Your pull request should address just this issue, without
    pulling in other changes.
  • Each commit in the pull request should have a meaningful subject line and body.
  • Format the pull request title like [MNG-XXX] SUMMARY, where you replace MNG-XXX
    and SUMMARY with the appropriate JIRA issue. Best practice is to use the JIRA issue
    title in the pull request title and in the first line of the commit message.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Run mvn clean verify to make sure basic checks pass. A more thorough check will
    be performed on your pull request automatically.
  • You have run the Core IT successfully.

If your pull request is about ~20 lines of code you don't need to sign an
Individual Contributor License Agreement if you are unsure
please ask on the developers list.

To make clear that you license your contribution under
the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.

@michael-o
Copy link
Member

Has this been tested with Windows PowerShell or PowerShell 7?

@JurrianFahner
Copy link
Author

Has this been tested with Windows PowerShell or PowerShell 7?

Mainly with powershell 7.3.0, but now also with powershell 5.1.22621.608.

The test is done by running mvn clean package on a simple project. It will not cover all use-cases for the scripts that have been changed. Is there an example project to check all of these?

@michael-o
Copy link
Member

This is at least what I have, although I work from Windows Terminal with PowerShell 7 only:

C:\Users\mosipov> $PSVersionTable

Name                           Value
----                           -----
PSVersion                      5.1.19041.1682
PSEdition                      Desktop
PSCompatibleVersions           {1.0, 2.0, 3.0, 4.0...}
BuildVersion                   10.0.19041.1682
CLRVersion                     4.0.30319.42000
WSManStackVersion              3.0
PSRemotingProtocolVersion      2.3
SerializationVersion           1.1.0.1

I'd expect that on a Windows CI Agent only the Windows PS will be used. Thus, this should be our minimum requirement.

@michael-o
Copy link
Member

Try the Maven Core ITs with this. Make sure that .cmd isn't included, just in case.

@JurrianFahner
Copy link
Author

JurrianFahner commented Feb 7, 2023

The CoreIT tests have run well.

@michael-o michael-o removed their request for review February 20, 2023 22:33
@JurrianFahner
Copy link
Author

@michael-o From my perspective this PR is ready. What's your perspective on this position?

@michael-o
Copy link
Member

@michael-o From my perspective this PR is ready. What's your perspective on this position?

I want to get back to this end of month.

@JurrianFahner
Copy link
Author

@michael-o Gentle reminder

@JurrianFahner
Copy link
Author

@Giovds @michael-o This branch contains the latest improvements from #982.

apache-maven/src/assembly/maven/bin/mvnDebug.ps1 Outdated Show resolved Hide resolved
apache-maven/src/assembly/shared/init.ps1 Outdated Show resolved Hide resolved
apache-maven/src/assembly/shared/mvnvalidate.ps1 Outdated Show resolved Hide resolved
apache-maven/src/assembly/shared/mvnvalidate.ps1 Outdated Show resolved Hide resolved
apache-maven/src/assembly/shared/run.ps1 Show resolved Hide resolved
apache-maven/src/assembly/shared/validate.ps1 Show resolved Hide resolved
#>

# set title
$Host.UI.RawUI.WindowTitle = $MyInvocation.MyCommand
Copy link
Contributor

Choose a reason for hiding this comment

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

This probably has the same 'issue' as as 3.x script: #982 (comment)?

@@ -0,0 +1,83 @@
# ==== END VALIDATION ====

$CLASSWORLDS_CONF = "$MAVEN_HOME\bin\m2.conf"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be part of mvnlauncher.ps1 as it is never used in this file?

Copy link
Member

Choose a reason for hiding this comment

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

I'd keep it identical to init.cmd.

Copy link
Contributor

@Giovds Giovds left a comment

Choose a reason for hiding this comment

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

It would be nice to fix the command terminal title, the rest LGTM

@JurrianFahner
Copy link
Author

JurrianFahner commented Oct 14, 2023

It would be nice to fix the command terminal title, the rest LGTM

@Giovds I've created an issue to improve the command terminal title: https://issues.apache.org/jira/browse/MNG-7907

The description is a little bit rough right now and needs some extra refinement.

apache-maven/src/assembly/maven/bin/mvnDebug.ps1 Outdated Show resolved Hide resolved
@@ -0,0 +1,83 @@
# ==== END VALIDATION ====

$CLASSWORLDS_CONF = "$MAVEN_HOME\bin\m2.conf"
Copy link
Member

Choose a reason for hiding this comment

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

I'd keep it identical to init.cmd.

if (Test-Path $jvm_config) {
return $env:MAVEN_OPTS + " " + -Join (Get-Content $jvm_config)
}
return $env:MAVEN_OPTS
Copy link
Member

Choose a reason for hiding this comment

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

That is not correct. If the file is empty then it is empty. See approach in init.

Copy link
Author

Choose a reason for hiding this comment

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

I don't understand your comment.

Copy link
Member

Choose a reason for hiding this comment

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

You are joining the content of the jvm.config file with MAVEN_OPTS variable in one guy. From a caller's perspective that should not be done and it is not obvious from where the config comes. Look at the original files, they do not merge them.

}
}

$MAVEN_OPTS = (RetrieveContentsJvmConfig $basedir)
Copy link
Member

Choose a reason for hiding this comment

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

See above.

# check if maven command exists
if (-not (Test-path "$MAVEN_HOME\bin\mvn.ps1")) {
Write-Error -Message "Maven command ($MAVEN_HOME\bin\mvn.ps1) cannot be found" -ErrorAction Stop
}
Copy link
Member

Choose a reason for hiding this comment

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

I wonder whether this is a relic from the past. In mvnvalidate this check isn't present and this file is part of mvn/mvn.cmd/mvn.ps1. How can this fail?`

Copy link
Author

Choose a reason for hiding this comment

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

If mvn.ps1 or mvn.cmd is removed or not accessible.

But it is indeed a corner case, shall we remove it?

Copy link
Member

Choose a reason for hiding this comment

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

But how can this happen at all? This is a chicken-and-egg issue. This block is part of the merged mvn.ps1. If the file is not present that check will never happen, no? This confuses me.

apache-maven/src/assembly/shared/validate.ps1 Show resolved Hide resolved
@michael-o michael-o self-requested a review April 28, 2024 09:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants