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

Restructure project to make testing branches easier #102

Merged
merged 10 commits into from
Mar 31, 2019

Conversation

dagwieers
Copy link
Collaborator

Ok, if we do this the GitHub ZIP file will simply work in Kodi.
Most (if not all) addons do it like this, so you can more easily test development versions.

@dagwieers
Copy link
Collaborator Author

@mediaminister I'd like your input on this one too :-)

@dagwieers dagwieers requested a review from pietje666 March 26, 2019 20:22
@dagwieers dagwieers force-pushed the restructure-project branch from 3079b6a to d484eec Compare March 26, 2019 20:24
@dagwieers
Copy link
Collaborator Author

Looking at the ZIP file, the directory seems to become plugin.video.vrt.nu-master so I am not entirely sure this is going to work. Cannot test now (watching something), will test later.

@pietje666
Copy link
Collaborator

Now if people make a zip they also make a zip of the packagemaker and sln files. And the packagemaker is now part of the python project. They were structured like before so there is a clear structuring between python code/ other. The structure actually also worked very nicely with my IDE (Visual Studio). Do you use an IDE @dagwieers ? When creating the python project so far i have been adding the top folder to create a new project containing all the python files.

@dagwieers
Copy link
Collaborator Author

My IDE is VIM, has been for the past 25 years ;-)

@dagwieers dagwieers added enhancement New feature or request question Further information is requested labels Mar 26, 2019
@dagwieers
Copy link
Collaborator Author

Let me test if this would work, if it doesn't there's not even an argument ;-)

@mediaminister
Copy link
Collaborator

I'm not against this change, cause I'm also using Linux. But breaking the workflow of pietje666 is definitely a no go.

@dagwieers
Copy link
Collaborator Author

So it does work, you can download any branch, or any tree based on any commit (like a PR) via: wget https://github.com/pietje666/plugin.video.vrt.nu/archive/d484eecfc7a21e172b6e751bdbf3a0889098ff84.zip and it ends up correctly as well.

I would have been surprised if it didn't because this is what I have been doing with other plugins for testing. Personally I think this a must-have if we want to get users involved (or get fixes available to users more easily). But I understand that this breaks @pietje666 's workflow. Tough one.

My hope was we could document this in the Wiki to explain how people can easily test, modify and troubleshoot issues. (Also improve logging statements and increase the Kodi log-level, etc.)

@pietje666
Copy link
Collaborator

Please don't! Then also you will have a zip containing lotsoff other files as well creating the zip by running your make file or the packagemaker will create a clean and concise zip without cluttering. (and i maintain my excellent structured folders :D). Can't you move all the python code one folder deeper like => Kodi Addon/plugin.video.vrt.nu/

Then you can easily make a zip manually from that folder. Im not sure if Github can make a a zip from a specific folder? I guess if it could work like this both world's are happy

@dagwieers
Copy link
Collaborator Author

@pietje666 Don't worry, I am not taking over this project ;-) If I cannot convince you it won't happen.

@dagwieers
Copy link
Collaborator Author

dagwieers commented Mar 26, 2019

Please don't! Then also you will have a zip containing lotsoff other files

Yeah, well that's the tradeoff for the convenience of having every branch and PR at your fingertips for testing. Just wget <link> onto your device and it would install.

Taking this a bit further, we could even offer a repository (a dynamic XML file pointing to these links) with every branch/tag/release/PR that people could add to their Kodi and they would be able to select any version directly from Kodi and report back. @mediaminister fixes a bug, and the user would be able to test from Kodi with a few clicks.

(None of the other addons have DLL's added for creating that ZIP file, if we would remove the zipmaker/packagemaker, there wouldn't be a lot of unwanted stuff in the GitHub ZIP file, maybe a few unneeded text files like tox.ini, .travis.yml or a Makefile, but those are small and harmless and are removed when you move back to a proper release anyway).

@michaelarnauts
Copy link
Contributor

For some plugins, I just git clone in the add-ons folder of Kodi so I can easily git update to have the latest code (can be trough Cron). This makes testing (and living on the edge :)) a breeze. I think this is a good thing...

@dagwieers dagwieers changed the title Restructure project Restructure project to make testing branches easier Mar 27, 2019
@dagwieers
Copy link
Collaborator Author

dagwieers commented Mar 27, 2019

The process to create a new ZIP package was already documented in the Wiki Testing page. This procedure could be replaced by a single statement if this PR would be merged.

I now also added an end-user Updating page detailing on how to get an update faster or update to a (specific) release manually.

@dagwieers dagwieers force-pushed the restructure-project branch 2 times, most recently from b2b5cbe to 8b5dc90 Compare March 27, 2019 11:27
@michaelarnauts
Copy link
Contributor

@pietje666 can you please consider the benefits over using a more standard packaging/release process?

One additional benedit could be to more easily release, since you could automate everything in a ci pipeline. This is much harder to do with an .net application that would require windows (or maybe mono).

@pietje666
Copy link
Collaborator

You could also use a tool like downgit => https://minhaskamal.github.io/DownGit/#/home?url=https:%2F%2Fgithub.com%2Fpietje666%2Fplugin.video.vrt.nu%2Ftree%2Fmaster%2Fplugin.video.vrt.nu for instance this downloads the plugin.video.vrt.nu folder on the master branch and creates a zip. I think you can also add the folder as a repository?

Anyway my goal is to create a release zip very easily. As @dagwieers said before this could also easily be achieved by scripting which is fine by me. Unfortunately scripting is far from my speciallity and i hate to do it (thats why i added the .net core packager). But if someone would provide a script for me (for windows) to replace the packagemaker, then i can deal with it.

The prerequisites for script are that it creates a clean zip with only the needed files (no *.pyc/.sln/pyproj,..) files for instance

@dagwieers
Copy link
Collaborator Author

@pietje666 I will make a Powershell script that implements this, no problem. But I am not 100% if this PR conforms to your IDE/project requirements as I am not familiar with Visual Studio. (I do have Visual Code on my system, if this is similar I can manage).

But again, I don't want to force this on you. I don't mind going the extra mile, but only if you support this going forward.

@dagwieers dagwieers force-pushed the restructure-project branch from f1bfbaf to 0d422a5 Compare March 28, 2019 02:03
@dagwieers
Copy link
Collaborator Author

dagwieers commented Mar 28, 2019

@pietje666 I added a basic PowerShell script. Should work fine on Windows. Using PowerShell Core 6:

[dag@moria plugin.video.vrt.nu]$ pwsh Build-Zip.ps1
= Building new package
<snip>
= Successfully wrote package as: ../plugin.video.vrt.nu-1.7.1-0d422a5.zip

If you have an older PowerShell you may need to run it either in PowerShell:

.\Build-Zip.ps1

or in another shell:

powershell.exe -ExecutionPolicy Bypass -File Build-Zip.ps1

The ZIP file is created in the current directory (for convenience).

@dagwieers dagwieers force-pushed the restructure-project branch 2 times, most recently from b67b217 to c484a0b Compare March 28, 2019 03:04
@pietje666
Copy link
Collaborator

The script gives me errors => first it gave an issue on
Add-Type -AssemblyName System.IO.Compression
so id changed it to Add-Type -AssemblyName System.IO.Compression.FileSystem which resolved that issue.

Then i get errors like this

Exception calling "Open" with "2" argument(s): "The file 'C:\Users\Martijn\plugin.video.vrt.nu-1.7.1-14c5add.zip'
already exists."
At C:\projects\dagwiers\plugin.video.vrt.nu\MakeZipWindows.ps1:23 char:1
+ $zip_file = [System.IO.Compression.ZipFile]::Open($zip_name, 'Create' ...
+ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    + CategoryInfo          : NotSpecified: (:) [], MethodInvocationException
    + FullyQualifiedErrorId : IOException

Exception calling "CreateEntryFromFile" with "3" argument(s): "Value cannot be null.
Parameter name: destination"
At C:\projects\dagwiers\plugin.video.vrt.nu\MakeZipWindows.ps1:26 char:5
+     [System.IO.Compression.ZipFileExtensions]::CreateEntryFromFile($z ...
+     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    + CategoryInfo          : NotSpecified: (:) [], MethodInvocationException
    + FullyQualifiedErrorId : ArgumentNullException

like it does not find the files, i execute it through powershell 5 like this

PS C:\projects\dagwiers\plugin.video.vrt.nu> .\MakeZipWindows.ps1

@dagwieers
Copy link
Collaborator Author

dagwieers commented Mar 29, 2019

I just discovered that if you run it as .\Build-Zip.ps1 it indeed gives this weird error.
But it works fine if you run it as:

powershell.exe -ExecutionPolicy Bypass -File Build-Zip.ps1

@dagwieers dagwieers force-pushed the restructure-project branch 2 times, most recently from 150d4e5 to 5c490e9 Compare March 29, 2019 02:03
@dagwieers
Copy link
Collaborator Author

Well, before I knew it I went down the rabbit hole very quickly, apparently if you use .NET objects in Powershell it may use a different working directory for the .NET objects then for the Powershell cmdlets. And so depending on how you run it, this causes the issue you have seen.

https://stackoverflow.com/questions/11246068/why-dont-net-objects-in-powershell-use-the-current-directory/16305221

Another silly Powershell design fuckup on that list, something I cannot unsee :-(
So the solution is to set the .NET current directory to Powershell's working directory.

Fixed.

@dagwieers dagwieers force-pushed the restructure-project branch from 5c490e9 to 9748b77 Compare March 29, 2019 12:05
@dagwieers
Copy link
Collaborator Author

Renamed MakeZipWindows.ps1 to Build-Zip.ps1 according to the Approved Verbs Microsoft Powershell standard. Also, it works on other platforms, not just Windows.

@pietje666
Copy link
Collaborator

There is something weird going on => when trying to install the zip created by powershell i get the error =Failed to read 'zip://C%3a%5cplugin.video.vrt.nu-1.7.1-9748b77.zip/plugin.video.vrt.nu/addon.xml'.

But when i unpack the zip and create a zip manually through windows and try to install it everything just works. (and its not zip name related tried to change that as well)

image

@dagwieers dagwieers force-pushed the restructure-project branch from 9748b77 to 296e24d Compare March 30, 2019 11:05
@dagwieers
Copy link
Collaborator Author

That is indeed weird. From the output of the .NET class, there are two things that are strange, although I wouldn't have suspected this behaviour:

  1. The Length and CompressedLength is empty.
Archive            : System.IO.Compression.ZipArchive
CompressedLength   : 
ExternalAttributes : 0
FullName           : plugin.video.vrt.nu/addon.xml
LastWriteTime      : 3/30/19 12:04:23 PM +01:00
Length             : 
Name               : addon.xml
  1. The joined paths have a path1/./path2 construct, which (at least on Linux) is not a real problem.
Archive            : System.IO.Compression.ZipArchive
CompressedLength   : 
ExternalAttributes : 0
FullName           : plugin.video.vrt.nu/./resources/language/resource.language.en_gb/strings.po
LastWriteTime      : 3/30/19 12:04:23 PM +01:00
Length             : 
Name               : strings.po

AFAIR the Linux ZIP file generated from PowerShell worked fine on Linux. I will test using Windows later.

@dagwieers dagwieers force-pushed the restructure-project branch from 296e24d to cc257ce Compare March 30, 2019 12:37
@dagwieers
Copy link
Collaborator Author

dagwieers commented Mar 30, 2019

The filename in your error looks weird: 'zip://C%3a%5cplugin.video.vrt.nu-1.7.1-9748b77.zip/plugin.video.vrt.nu/addon.xml'

C%3a%5c and then forward slashes is plain weird.
plugin.video.vrt.nu-1.7.1-.zip
plugin.video.vrt.nu-1.7.1-2.zip
plugin.video.vrt.nu-1.7.1-3.zip

@dagwieers
Copy link
Collaborator Author

dagwieers commented Mar 30, 2019

Ok, I fixed it. So the ZIP files build on Linux (using Powershell) worked fine on Linux and Windows, but the ZIP files build on Windows failed on Linux and Windows. I ran unzip -l on a Linux and Windows-generated ZIP file, and the difference was:
Linux:

2177  03-30-2019 12:04   plugin.video.vrt.nu/addon.py

Windows:

2177  03-30-2019 22:40   plugin.video.vrt.nu\addon.py

So the solution was to replace \ with /. You would think the path-separator was a thing from the past, but it seems it's alive and kicking in ZIP files. And Kodi really dislikes the Windows-variant.

(It also means .NET ZipArchive class is lying when it outputs the path)

Fixed.

@pietje666
Copy link
Collaborator

Can confirm it works well now! Thanks for providing the script

@dagwieers dagwieers merged commit 82f401d into add-ons:master Mar 31, 2019
@dagwieers
Copy link
Collaborator Author

dagwieers commented Mar 31, 2019

Thanks everyone, the tree looks now like this:

|-- resources
|   |-- language
|   |   |-- resource.language.en_gb
|   |   `-- resource.language.nl_nl
|   |-- lib
|   |   |-- helperobjects
|   |   |-- kodiwrappers
|   |   `-- vrtplayer
|   `-- media
`-- test

And creating a ZIP file from any branch within GitHub works as a package in Kodi.
I will document it as such.

@dagwieers
Copy link
Collaborator Author

dagwieers commented Mar 31, 2019

I updated the Wiki documentation with instructions how to download the latest development version (master branch) or any other branch. Also for end-users in Updating.

Feel free to share this information with others when fixing issues and asking for feedback.

@dagwieers dagwieers added this to the 1.8.0 milestone May 3, 2019
@dagwieers dagwieers added testing Related to automated testing and removed enhancement New feature or request labels Sep 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested testing Related to automated testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants