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

(nuget.commandline) AU updater #682

Merged
merged 2 commits into from
Apr 17, 2017
Merged

(nuget.commandline) AU updater #682

merged 2 commits into from
Apr 17, 2017

Conversation

majkinetor
Copy link
Contributor

For @ferventcoder

closes #681

au_GetLatest is generic, can work with any package, just change the $packageName.

@majkinetor majkinetor changed the title (nuget.commandline) AU udpater (nuget.commandline) AU updater Apr 17, 2017
@@ -0,0 +1,27 @@
<?xml version="1.0" encoding="utf-8"?>
<!-- Do not remove this test for UTF-8: if “Ω” doesn’t appear as greek uppercase omega letter enclosed in quotation marks, you should use an editor that supports UTF-8, not this one. -->
Copy link
Member

Choose a reason for hiding this comment

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

you got alot of bogus characters here.
Please replace them with “Ω”

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They pile up after few forced updates

Copy link
Member

Choose a reason for hiding this comment

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

If you haven't fixed the character after I submitted majkinetor/au#63, then that would be expected.
If you remove them and replace it with the omega character, then it shouldn't continue to pile up like that.

</description>
<projectUrl>https://github.com/NuGet/NuGet.Client</projectUrl>
<tags>nuget dot.net microsoft foss cross-platform</tags>
<copyright>© .NET Foundation. All rights reserved.</copyright>
Copy link
Member

Choose a reason for hiding this comment

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

Did you really mean to have the  character here?

@majkinetor
Copy link
Contributor Author

AU does that. I think after majkinetor/au#60

I dont understand why. Inever got to the point where encoding behaves correctly, there is always someone who complains...

@AdmiringWorm
Copy link
Member

@majkinetor yes that's when it started to happen, but it shouldn't anymore (as long as the bogus characters are fixed before the next update).

Basically it happened because the character was read as an ANSI character (although it's utf-8) and then converted to UTF-8 BOM.
In that case it didn't know before it converted it what kind of character it was.

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.

LGTM

@majkinetor
Copy link
Contributor Author

but it shouldn't anymore

I am not sure I can confirm that. But will take another look.

@AdmiringWorm
Copy link
Member

I am not sure I can confirm that. But will take another look.

If you try to run test_all.ps1 again for this package now, you could confirm or deny my claim.
I've seen though, after I submitted the fix it never again happened to me.

@majkinetor
Copy link
Contributor Author

Looks like appveyor check works great BTW.

One thing I noticed, screenshots are black.

@majkinetor majkinetor merged commit cd8b20e into master Apr 17, 2017
@majkinetor majkinetor deleted the nuget.commandline branch April 17, 2017 14:58
@AdmiringWorm
Copy link
Member

One thing I noticed, screenshots are black.

Hmm, I'll need to look into that.
I know it worked great when an Image wasn't specified in appveyor.yml.

Seems like there is some incompatibilities between the vs2015 and WMF 5 image.

@majkinetor
Copy link
Contributor Author

Not that it is terribly important IMO. Its probably equally effective to just return desktop content as an artifact - will show up links and anything new added.

Screenshots might come in handy with a more robust check procedure - i.e. starting the GUI programs after install to see if they show up window. This could be added too , perhaps as as a directive to AV or something. Perhaps as a new PS1 script (verify_install.ps1) that branch would run after choco install ?

@AdmiringWorm
Copy link
Member

@majkinetor although not terribly important, the purpose of the screenshots is to see if a program is started after the install, or is hanging on the uninstall, etc.

@AdmiringWorm
Copy link
Member

I've disabled the screenshots until I can find a working way with the WMF 5 image.

@majkinetor
Copy link
Contributor Author

Something is wrong now that this got merged. During branch testing everything was fine.

There is nothing in au temp folder which is very strange.

Only this in build log:

Unable to find icon url for nuget.commandline
    + CategoryInfo          : OperationStopped: (Unable to find ...get.commandline:String) [], RuntimeException
    + FullyQualifiedErrorId : Unable to find icon url for nuget.commandline

And this in info:

PS> Import-Clixml .\update_info.xml | % result | % errors
Path          : C:\projects\chocolatey-coreteampackages\automatic\nuget.commandline
Name          : nuget.commandline
Updated       : False
Pushed        : False
RemoteVersion :
NuspecVersion : 0
Result        :
Error         : Job returned no object, Vector smash ?
NuspecPath    : C:\projects\chocolatey-coreteampackages\automatic\nuget.commandline\nuget.commandline.nuspec
NuspecXml     : #document

Do you spot anything unusual ?

@AdmiringWorm
Copy link
Member

@majkinetor ah damn, I missed that.
It's because the Update-IconUrl script was updated to allow skipping packages (and thus changed to throw an error when one isn't found).
I didn't notice you didn't drop the nuget.commandline icon in the icons directory.
Just drop the icon in that directory, or add a comment in the nuspec file with the text Icon: Skip or IconUrl: Skip check

@AdmiringWorm
Copy link
Member

I'll update the script mentioning how to skip the icon check for a package in the error message

@majkinetor
Copy link
Contributor Author

OK, so it was responsible for this behavior ? I'll add a comment since there is no icon.

BTW


pdating 1 automatic packages at 2017-04-17 14:57:39 (forced)
Push is disabled
FORCE IS ENABLED. All packages will be updated
Invalid nuspec file Version '0' - using 0.0
   nuget.commandline is updated to 3.5.0
Running Report
Saving markdown report: C:\projects\chocolatey-coreteampackages\Update-Force-Test-.md
Running Gist
  ERROR:  
    {"message":"Not Found","documentation_url":"https://developer.github.com/v3/gists/#edit-a-gist"}

Maybe it would not be bad to actually see report on branch au test ? We could create a new gist for that and set a link in main report via UserMessage, so we can quickly see full AU results... Just food for thought.

@majkinetor
Copy link
Contributor Author

Should it be a single comment anywhwere, or inside existing comment or what ?

@AdmiringWorm
Copy link
Member

it doesn't matter if it's in a single comment or not. It actually don't even need to be inside a comment.
I'm just running each line in the nuspec file through the following regex: Icon(Url)?:\s*Skip( check)?

@AdmiringWorm
Copy link
Member

I was sure that nuget had an icon though.
If it doesn't probably iconUrl should be removed from the nuspec file

@AdmiringWorm
Copy link
Member

perhaps reuse the icon that was used when we tested out using nuget.portable package.
https://github.com/pascalberger/chocolatey-coreteampackages/blob/302a1ba6bf175cc37fd23b13a131a034385fca88/icons/nuget.png

@majkinetor
Copy link
Contributor Author

Perhaps you can add an error to info object on particular package when you fail iconurl check, then it will be visible in the report.

@AdmiringWorm
Copy link
Member

Sure, I'll look into doing that.

@majkinetor
Copy link
Contributor Author

I think if you return AUPackage object and set pkg.Error it will show. I am not sure if you can actually create an object, perhaps I need to expose it as a public builder function, you will have to try it.

$pkg = [AUPackage]::new(  'packagename' )
$pkg.Error = 'failed icon check'

@majkinetor
Copy link
Contributor Author

Hm... not even that will work, the routine will return that package only if it goes to the end.

I will see how can I change AU to allow for before and after each scripts to to this.

So postpone your work until majkinetor/au#73 gets implemented.

@AdmiringWorm
Copy link
Member

👍 will do, hadn't started yet as the food was just ready.

but what about, perhaps catching thrown errors in AU?

@majkinetor
Copy link
Contributor Author

Yeah will see to do that, but perhaps access to the AUPackage is still desirable because as scripts could add arbitrary text in results array that will show in report

@AdmiringWorm
Copy link
Member

yeah, I think that would be fine as well.

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.

(nuget.commandline) new package
2 participants