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

get builds running on linux #271

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

phillip-haydon
Copy link

@phillip-haydon phillip-haydon commented Jul 30, 2020

  • Added github action workflow to build the project on Linux (ubuntu-latest) and Windows (windows-latest)
  • Based on Fody targeting .NET Framework 4.5.2, changed 4.7.2 to 4.5.2
    • The Nuget still creates with 4.5.2 and NETStandard 1.0 as targets
  • README.md > readme.md (linux is case sensitive so it couldn't find the file.)
  • Tests target netcoreapp3.1
    • I can target 2.1 also if need be? Not sure if there's any value in building against 452.

I also deleted this file: MethodDecorator.Fody.PnP.Tests/AssemblyExtensions.cs

As the project has a link-file to the implementation in MethodDecorator.Fody.Tests so it resulted in 2 versions in there which didn't make sense.

  • Related issues
  • Tests
  • Documentation

Copy link
Member

@ltrzesniewski ltrzesniewski left a comment

Choose a reason for hiding this comment

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

Thanks! I left a couple comments, but it's up to @SimonCropp to decide if he wants these changes, and if we should move to GitHub actions more generally.

@@ -0,0 +1,42 @@
name: on-push-run-build
on:
push:
Copy link
Member

Choose a reason for hiding this comment

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

Something like on: [push, pull_request] would be needed to handle PRs.

Copy link
Author

Choose a reason for hiding this comment

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

https://docs.github.com/en/actions/reference/workflow-syntax-for-github-actions#example-using-a-list-of-events

Checking this I think you're right.

I didn't realise appveyor was there because I saw there were already github actions for tagging and doco.

But this also allows other contributors to enable actions on their fork and test their work too.


env:
# Opt out of telemetry
DOTNET_CLI_TELEMETRY_OPTOUT: 1
Copy link
Member

Choose a reason for hiding this comment

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

Consider adding DOTNET_NOLOGO: 1 to avoid the spam in the console.

Copy link
Author

Choose a reason for hiding this comment

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

I didn't know about this. I've included it. Thanks.

@@ -7,7 +7,7 @@
<Authors>Simon Cropp, Matt Ellis, Alexey Suvorov, Andy Hoyle, Tony (@megafinz), and contributors</Authors>
<Description>Fody add-in to decorate arbitrary methods to run code before and after invocation.</Description>
<PackageTags>ILWeaving Fody AOP</PackageTags>
<NoWarn>CS1591;NU5228</NoWarn>
<NoWarn>CS1591;NU5228;NU5118</NoWarn>
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't that hide an issue?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, so this warning is actually defined in Directory.Build.props. Because the project is set to treat all warnings as errors, when the weaving happens for the generated dll, it causes it to error out because the dll already exists.

What we want is it to ignore the warning and override the dll before packaging.

At least that's my understanding.

When I run the build in Rider or Command Line, it throws NU5118, if it's in Visual Studio on Windows it doesn't appear to throw as I believe it picks up the setting from Directory.Build.props

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.

3 participants