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

(msys2) fix #1286 for non-interactive installs for msys2 #1296

Closed
wants to merge 1 commit into from
Closed

(msys2) fix #1286 for non-interactive installs for msys2 #1296

wants to merge 1 commit into from

Conversation

diablodale
Copy link

Description

  • fix (msys2) Currently failing the verifier #1286
  • fix is isolated to only address the problem with non-interactive installed. I did not refactor code. I did not fix various minor problems I saw elsewhere. I made no changes to metadata or other fields described in the CONTRIBUTING document.
  • Included a single other fix of a horrid bug that deletes all *.xz files from the root directory on the hard drive.
  • The particular approach the original coder used within the semver framework is unfortunate. They used the first xxx number (xxx.yyy.zzz) as the often iterated 3rd party field, and the next two fields unused. However, to maintain version continuity, I made no changes to the approach and instead incremented the 3rd field by 1. So with this fix, the versions will be (xxx.0.1)

Motivation and Context

How Has this Been Tested?

  • Tested using Docker with clean Windows 1809 and Powershell 5.1
  • Used the AU package to create the nupackage and update the scripted files
  • I encourage someone with test harnesses to test on more platforms.
  • Confirmed that chocolatey doesn't work with powershell (core) 6+
  • Unable to test with chocolatey-test-environment due to using hyperv provider, can not read files in /packages share chocolatey-test-environment#41
  • Attempted to use the -version 2 param on powershell, but it requires a matching v2 .netframework which is not available in the Microsoft provided docker containers. I do not have other sources of old Microsoft OS's.

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 the already existing code in the changed files. No new files added.
  • 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).
  • 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).

* fix #1286
* minimal set of changes, no refactoring
@AppVeyorBot
Copy link

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

@diablodale
Copy link
Author

Update...I was able to get chocolatey-test-environment working on an old laptop using vbox provider.
This PR successfully installed when vagrant provision was run with a line in it as follows:

choco.exe install -dy msys2 --source "'c:\\packages;https://chocolatey.org/api/v2/'"

This PR made no changes in uninstall behavior. I ran uninstall with vagrant provision and the below line and encountered no visible errors.

choco.exe uninstall -dy msys2

Please note, chocolatey-test-environment provides powershell 4.0. Therefore, there is no test coverage for powershell 3.0.

@majkinetor
Copy link
Contributor

I must test this and I am unable to do this now

Looks legit anyway, but few comments:

  • invocation of musys2 shell deserves its own function in helper script (i.e. see 1.1.7 in CONTRIBUTING.md).
  • you can fix other things you noticed too
  • we have package fix notation that you should use instead adding 1 instead zero.

@majkinetor
Copy link
Contributor

And thanks for noticing that horrid error I did 🍦 .
It probably never made any real harm since keeping xz files in system root isn't really a thing, but that doesn't make it much less horrid.

@diablodale
Copy link
Author

Hi. I see no changes that I would make at this time.

No additional work

This is a specific bug fix. I will make no changes to code outside fixing this specific bug.

🙂 If you see or find bugs with the isolated code that was changed, please do provide feedback on it. I committed to do this specific bug fix and I'm very willing to do the needed coding and testing to fix this specific issue.

I did not make a commitment to do work outside this specific bug. Your suggestions of code overhaul, refactoring, minor seen bugs, etc. is work I did not commit to do. I do not have the bandwidth to do that dev and test work at this time.

Versioning

I believe the semver does need to have an incremented "1" in the 3rd field. I have already done that in this PR's code. I do not believe the semver needs additional suffixes or a 4th segment. This leads to the diaper syndrome "Pampers Super Ultra Absorbent Deluxe Overnight Large" which provides no useful information, creates clutter, and more parsing challenges for users (e.g. 1997.2.11-beta.1.6+exp.sha.5114f85) The existing notation on this package of DATE.0.0 provides no useful information in the 2nd or 3rd fields and the 2nd and 3rd fields will otherwise never be used. Msys itself does not use semver. Their version notation is a single 8 number string which has multiple years of legacy that prevents their team from changing.

I did read again https://chocolatey.org/docs/create-packages#package-fix-version-notation. I believe the recommendations there do not fit well to this package and software.

  • "So if the software is 1.1.0" Msys is instead "20190524". No semver.
  • "if an application only uses 1 or 2 version segments, add zeros until you get to the 4th segment"
  • If you find that the 1.1.0 package has an issue and you need to fix the package but keep the same version of the software, that is where package fix version notation comes into play. You would end up with both a 1.1.0 package and a 1.1.0.YYYYMMDD

That leads to version possibilities like 20190524.0.0.20190621
or 20190621.0.0.20190621
or worse 20180531.0.0.20190524
The last example is one where both dates are valid msys versions. And someone also made a package update on one of those dates. So there is no way a user can know which date is the package or the software without reading the package's code in git. 😟 Because msys has its own date (and choco suggests its own independent date), the dates can be in different orders, sequences, or the same. This is confusing and difficult for users. A computer can understand it. But not a human.

If you want me to change line 32 of automatic/msys2/update.ps1 to have a date in a new fourth segment, I will 🙂 do that for you under duress and the above objections. Please tell me the date you want me to insert in a new 4th field instead of the "1" in the third field as the date is always changing.

@majkinetor
Copy link
Contributor

This is a specific bug fix. I will make no changes to code outside fixing this specific bug.

Yet you did fix horrific bug that has nothing to do with it.

I did not make a commitment to do work outside this specific bug.

Nobody here commits to anything.

Regarding your semversion ideas, not sure what can I say about it. All packages here use the same standard which is well known by users by now and AU module also enforces it.

If you want me to change line 32 of automatic/msys2/update.ps1 to have a date in a new fourth segment, I will 🙂 do that for you under duress and the above objections.

Do that for me ? Please don't, I don't need anybody doing stuff for me.
There are rules here we as a team devised. There is no 'me' here. Personally I don't like chity chat where fix is due, its easier then to fix problem myself. I wrote the original package proper where none existed, when I needed one, for me and the community (since I was already in the relevant topic, which as you can see requires some non-trivial amount of time). Due to he upstream changes something became broken (as far as I can see from your changes some program hangs in the background). This should be communicated better in a helper function (and DRY). Any package changes here should be acording to current guideliness, or we will quickly deteoriarate into chaos and madness (like before).

@diablodale
Copy link
Author

No answers yet from the Chocolatey team. I'm cleaning up loose ends that have no progress and this qualifies. If there is no progress, I'll delete this PR in one to two weeks.

@diablodale
Copy link
Author

No progress from core team. Aborting PR.

@diablodale diablodale closed this Jul 21, 2019
@mkevenaar
Copy link
Member

@diabodale I think core team has other prios right now. They will probably verify and merge this when they are ready for the next release.

I would suggest to reopen this PR and wait for their feedback.

@floh96
Copy link

floh96 commented Sep 10, 2019

@diablodale Please reopen the PR and wait for feedback

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.

(msys2) Currently failing the verifier
5 participants