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

Allow use of build module on unknown Linux distros #11146

Merged
merged 3 commits into from
May 29, 2020
Merged

Allow use of build module on unknown Linux distros #11146

merged 3 commits into from
May 29, 2020

Conversation

Saancreed
Copy link
Contributor

PR Summary

The build module build.psm1 currently throws an error when imported on unrecognized Linux distributions. This PR allows the module to be used in such cases, while preserving the "not supported" message as warning.

PR Context

Building PowerShell Core on distros like Arch Linux, while unsupported by Microsoft / PowerShell Team, is already possible as long as build dependencies are provided by the user. In such cases, the most simple way to do so would be to skip the toolchain bootstrapping steps as outlined here and ensure that they are satisfied by distro-specific tools (Arch: a PKGBUILD with depends and makedepends ensures that necessary runtime and/or build packages are provided by pacman). After that, we could use build.psm1 to build PowerShell from source, which is currently not possible because of hard exception thrown from Get-EnvironmentInformation function, even though the build could still continue and succeed as normal.

PR Checklist

@TravisEz13
Copy link
Member

What family of distro's does this work with?

@Saancreed
Copy link
Contributor Author

Arch Linux at the very least (in fact, community-maintained Arch packages are mentioned here, but they are being built manually), probably also its derivatives. And I suppose some Gentoo people could also make use of this.

I can successfully build PowerShell on my Arch PC with this change when using the build module.

@TravisEz13
Copy link
Member

@Saancreed some feature that rely on native component may not work, such as WinRM based remoting.

@vexx32
Copy link
Collaborator

vexx32 commented Nov 21, 2019

@TravisEz13 for which I think it's fair to warn. But I don't see a specific need to hamper efforts to expand PS's possibilities on "not supported" architectures. The warning message seems appropriate and still allows enterprising users the ability to at least try despite the potential for some missing / broken bits and pieces. 🙂

@Saancreed
Copy link
Contributor Author

Saancreed commented Nov 21, 2019

@TravisEz13 That is true, and frankly I'm not sure if I can somehow check that. But if some features are indeed broken, then using binaries built and linked against whatever glibc and openssl some distros currently use can help more than it can hurt.

@vexx32 Yep, if Arch users can make (parts of) PowerShell work that it's worth to make an AUR package, then they will use it even if some things don't work.

@iSazonov
Copy link
Collaborator

@Saancreed Thanks for your contribution!

I think we need to add new explicit switch parameter (-SkipLinuxDistroCheck with a comment that it is not supported) to enable the scenario without a risk to break supported build scenario.

@Saancreed
Copy link
Contributor Author

@iSazonov Supported build scenarios shouldn't be affected by this change at all, so introducing extra switch seems redundant (and makes the change not as simple as I orginally intended). The message itself is still printed to the console as warning, which preserves its visibility but still makes it clear to the user that building in such environments is not officially supported.

That said, if PowerShell team prefers a switch over succeeding with a warning, I can try and rewrite this PR into something like that.

@iSazonov
Copy link
Collaborator

We change Build.psm1 module frequently. I believe the new switch will protect both the proposed scenario and regular build scenarios too. Also for the switch we could add a comment that the scenario is not supported and not recommended.
@TravisEz13 Thoughts?

@TravisEz13
Copy link
Member

I tend to agree with @iSazonov . The error message can tell you about the switch to bypass the check.

@Saancreed
Copy link
Contributor Author

@iSazonov @TravisEz13 Sounds fair, but one issue I can see is that the error is thrown when the module is imported, and there we can pass arguments only with -ArgumentList which is not very self-descriptive. Would leaving the warning on import and adding said switch to Start-PSBuild (and perhaps some other functions as well?) that causes the function to throw the error instead be an acceptable solution?

@TravisEz13 TravisEz13 added this to the 7.1.0-preview.1 milestone Nov 23, 2019
@ghost ghost assigned TravisEz13 Nov 23, 2019
@iSazonov
Copy link
Collaborator

You could update the message to inform user how bypass. So if we import the module without the argument we get throw and the message ("use ipmo .\Build.psm1 -ArgumentList SkipLinuxDistroCheck"). If we import with the argument we don't throw and get only warning that the scenario is not supported.

@Saancreed
Copy link
Contributor Author

@TravisEz13 @iSazonov Any thoughts on the second iteration of this PR?

@iSazonov iSazonov added the CL-BuildPackaging Indicates that a PR should be marked as a build or packaging change in the Change Log label Nov 27, 2019
@TravisEz13
Copy link
Member

restarted static analysis

@TravisEz13
Copy link
Member

@PoshChan Please remind me in 1 day

@PoshChan
Copy link
Collaborator

PoshChan commented Dec 9, 2019

@TravisEz13, this is the reminder you requested 1 day ago

@TravisEz13
Copy link
Member

@PoshChan Please remind me in 5 days

@PoshChan
Copy link
Collaborator

@TravisEz13, this is the reminder you requested 5 days ago

@TravisEz13 TravisEz13 removed this from the 7.1.0-preview.3 milestone May 14, 2020
@TravisEz13 TravisEz13 added this to the 7.1.0-preview.4 milestone May 14, 2020
@ghost ghost added the Review - Needed The PR is being reviewed label May 27, 2020
@ghost
Copy link

ghost commented May 27, 2020

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Mainainer, Please provide feedback and/or mark it as Waiting on Author

@iSazonov
Copy link
Collaborator

@Saancreed Please resolve merge conflicts.

@TravisEz13 The PR is waiting your review.

@TravisEz13 TravisEz13 added Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept and removed Review - Needed The PR is being reviewed Review - Waiting on Author labels May 27, 2020
@ghost ghost removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label May 28, 2020
@TravisEz13 TravisEz13 merged commit 9e0b940 into PowerShell:master May 29, 2020
@ghost
Copy link

ghost commented Jun 25, 2020

🎉v7.1.0-preview.4 has been released which incorporates this pull request.:tada:

Handy links:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CL-BuildPackaging Indicates that a PR should be marked as a build or packaging change in the Change Log
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants