-
-
Notifications
You must be signed in to change notification settings - Fork 94
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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: |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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> |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
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.