Skip to content

Conversation

@JohnTheGr8
Copy link
Member

@JohnTheGr8 JohnTheGr8 commented May 4, 2020

Changes

Plugin project packing:

  • moved the package properties (id, authors, etc) from nuspec to the project file
  • the nuget package is produced on release builds automatically (along with a symbols file)

AppVeyor:

  • the main project files are patched with the build version
  • the nuget packages for the plugin are packed as artifacts
  • updated the custom post-build script

Considerations for discussion

Versioning:
How do we want to deal with versioning? The current way used by Wox is to use x.x.{build} as a version that applies to all projects (the launcher itself, the plugin project, and all default plugins). This leads to subsequent releases having a big jump in the version number (for example v1.3.981 -> v1.3.996). If we want to change to a different versioning scheme, we should probably decide this now.

Squirrel:
Are we going to use Squirrel for installation/updates? I have not looked into it much, but it seems to not behave too well with .net core.

TODO

  • integrate sourcelink for the plugin project
  • remove AssemblyInfo files?

(related to #31)

JohnTheGr8 added 8 commits May 4, 2020 12:47
* use dotnet pack for the plugin project
* refactor Zip-Release and Pack-Squirrel-Installer functions
* remove reference to NuGet.CommandLine - no longer needed
also use the new format for package symbols (snupkg)
and remove the pack step from the post-build script (no longer needed)
@JohnTheGr8 JohnTheGr8 added the enhancement New feature or request label May 4, 2020
@JohnTheGr8 JohnTheGr8 self-assigned this May 4, 2020
SysC0mp
SysC0mp previously approved these changes May 4, 2020
@SysC0mp SysC0mp self-requested a review May 4, 2020 21:58
@SysC0mp
Copy link
Member

SysC0mp commented May 4, 2020

Accidentally approved the wrong pull request, sorry ...

@jjw24
Copy link
Member

jjw24 commented May 5, 2020

Considerations for discussion

Versioning:
How do we want to deal with versioning? The current way used by Wox is to use x.x.{build} as a version that applies to all projects (the launcher itself, the plugin project, and all default plugins). This leads to subsequent releases having a big jump in the version number (for example v1.3.981 -> v1.3.996). If we want to change to a different versioning scheme, we should probably decide this now.

Good idea. Any suggestions on this?

I personally would rather not have such big jumps in version number if possible, but I am not too across how AppVeyor manage versioning. Major.Features.Bugfixes is a good indicator for non-dev users.

Squirrel:
Are we going to use Squirrel for installation/updates? I have not looked into it much, but it seems to not behave too well with .net core.

Squirrel works fine with updating the app. The only temporary annoyance I can think of is the self contained publish is fairly large so need to add a splash screen. But overall I am personally ok with Squirrel.

BTW It is also used to provide portable mode.

@JohnTheGr8
Copy link
Member Author

Good idea. Any suggestions on this?

I personally would rather not have such big jumps in version number if possible, but I am not too across how AppVeyor manage versioning. Major.Features.Bugfixes is a good indicator for non-dev users.

We can customize how AppVeyor manages versioning to best suit our needs. I agree with you about using semantic version and avoiding the confusing version jumps.

An important question here is: do we want to manage automated releases with AppVeyor? I am not familiar with Github actions, but perhaps the majority of the @Flow-Launcher/team is, in which case we should probably pick that instead.

As for my workflow suggestions: I agree with what @theClueless suggested in jjw24/Wox#127

  • merges to master will deploy releases to github
  • merges to dev will deploy pre-releases

Additionally:

  • merges to master tagged as plugin will also deploy the plugin to nuget as a release
  • merges to dev tagged as plugin will also deploy the plugin to nuget as a pre-release

@jjw24
Copy link
Member

jjw24 commented May 5, 2020

An important question here is: do we want to manage automated releases with AppVeyor? I am not familiar with Github actions, but perhaps the majority of the @Flow-Launcher/team is, in which case we should probably pick that instead.

Automated releases is definitely the way forward.

Maybe see if @Flow-Launcher/team has a preference. I personally OK with either, as far as I know Azure DevOps is easy to set up with Github actions and the template is ready for build (but not for versioning). So either way is fine.

@NickSeagull
Copy link

I also agree on using semantic versioning, it is quite well accepted everywhere.

JohnTheGr8 added 3 commits May 8, 2020 23:07
* make the file easier to read
* minimal build output verbosity
* skip commits that edit markdown files
@JohnTheGr8 JohnTheGr8 marked this pull request as ready for review May 8, 2020 21:26
@JohnTheGr8
Copy link
Member Author

JohnTheGr8 commented May 8, 2020

Marking this ready for review so that, once we merge it, other commits/PRs are built and tested without errors.

I will open a separate PR to add deployment logic.

$o = "$p\Output\Packages"
Validate-Directory $o
# making version static as multiple versions can exist in the nuget folder and in the case a breaking change is introduced.
New-Alias Nuget $env:USERPROFILE\.nuget\packages\NuGet.CommandLine\5.4.0\tools\NuGet.exe -Force
Copy link
Member

@jjw24 jjw24 May 11, 2020

Choose a reason for hiding this comment

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

Nuget comandline is needed for packing and releasify with Squirrel, which outputs exe and also for updating

Copy link
Member

@jjw24 jjw24 May 11, 2020

Choose a reason for hiding this comment

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

ok I can see AppVeyor has built the packages correctly.
image

Doesnt look like it's a self contained build judging on the size:
image

And what is the zip file for?

Copy link
Member Author

Choose a reason for hiding this comment

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

  • NuGet.CommandLine is an outdated way of using nuget, AppVeyor has it as a CLI tool and your dev environment most likely has it as well.
  • self-contained executables are runtime-specific.... how will we deal with that? Will we offer different exe downloads for every target platform? What about Squirrel and update artifacts?
  • The zip file is just the release folder compressed. Wox did that on every build, I saw no reason to remove it...

Copy link
Member

@jjw24 jjw24 May 11, 2020

Choose a reason for hiding this comment

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

  • NuGet.CommandLine is an outdated way of using nuget, AppVeyor has it as a CLI tool and your dev environment most likely has it as well.

OK fair enough.

  • self-contained executables are runtime-specific.... how will we deal with that? Will we offer different exe downloads for every target platform? What about Squirrel and update artifacts?

Self contained can be platform and runtime indepedent, this way users wont have to manually download the Core runtime. https://docs.microsoft.com/en-us/dotnet/core/deploying/deploy-with-cli

RID: win7-x64 should cover Win 7 and above. Eg: dotnet publish ..\Flow.Launcher.sln -c Release -r win7-x64 --self-contained true --output ..\Output\Release

Otherwise for users who do have Core runtime or are on Win 7/8 will not be able to use it without manually go to website to install.

  • The zip file is just the release folder compressed. Wox did that on every build, I saw no reason to remove it...

OK.

Copy link
Member

Choose a reason for hiding this comment

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

What about Squirrel and update artifacts?

Squirrel update should be fine as long as these three is published in Releases:
Output\Packages\Flow-Launcher-1.0.0-full.nupkg
Output\Packages\Flow-Launcher-v1.0.0.exe
Output\Packages\RELEASES

also convert indentation to spaces
@jjw24
Copy link
Member

jjw24 commented May 11, 2020

Downloaded and installed the artifact(5ed5d52), getting this error and flow doesnt start:
"
Description: A .NET Core application failed.
Application: Flow.Launcher.exe
Path: C:\Users\Blah\AppData\Local\Flow\app-1.0.0\Flow.Launcher.exe
Message: Error:
An assembly specified in the application dependencies manifest (Flow.Launcher.deps.json) was not found:
package: 'PropertyChanged.Fody', version: '3.2.8'
path: 'lib/netstandard1.0/PropertyChanged.dll'
"

@JohnTheGr8
Copy link
Member Author

Downloaded and installed the artifact(5ed5d52), getting this error and flow doesnt start:
"
Description: A .NET Core application failed.
Application: Flow.Launcher.exe
Path: C:\Users\Blah\AppData\Local\Flow\app-1.0.0\Flow.Launcher.exe
Message: Error:
An assembly specified in the application dependencies manifest (Flow.Launcher.deps.json) was not found:
package: 'PropertyChanged.Fody', version: '3.2.8'
path: 'lib/netstandard1.0/PropertyChanged.dll'
"

Probably because of:

<PrivateAssets>all</PrivateAssets>
<IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>

But from a quick try, I could not reproduce. Will look into it tomorrow...

@jjw24
Copy link
Member

jjw24 commented May 18, 2020

Downloaded and installed the artifact(5ed5d52), getting this error and flow doesnt start:
"
Description: A .NET Core application failed.
Application: Flow.Launcher.exe
Path: C:\Users\Blah\AppData\Local\Flow\app-1.0.0\Flow.Launcher.exe
Message: Error:
An assembly specified in the application dependencies manifest (Flow.Launcher.deps.json) was not found:
package: 'PropertyChanged.Fody', version: '3.2.8'
path: 'lib/netstandard1.0/PropertyChanged.dll'
"

Probably because of:

<PrivateAssets>all</PrivateAssets>
<IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>

But from a quick try, I could not reproduce. Will look into it tomorrow...

I am able to replicate the error with Dev build- delete %userprofile%\.nuget\packages\propertychanged.fody folder. Hope this helps. I will put in a PR to reduce the PropertyChange.Fody version for now until we can properly fix.

This is the latest version of the package that copies itself to the
build output. Downgrading fixes runtime errors.
The squirrel artifats produced (nupkg, RELEASES and exe) are not ready
for deployment as they depend on the .net core runtime being installed.
We remove them temporarily from the build artifacts to avoid confusion,
until this issue has been addressed.
The way the project files are currently configured, this setting
resulted in only the Plugin project being patched. Consequently, the
version of the Plugin project was wrong in the built package.
@JohnTheGr8
Copy link
Member Author

@Flow-Launcher/team

In order to get this PR merged sooner rather than later, and have working automated builds for the project in the meantime, I have limited the scope of this PR.

I have encountered various issues working on this PR that I will try to describe in separate issues and address individually in future PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants