-
Notifications
You must be signed in to change notification settings - Fork 100
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
Refac build configs #846
Refac build configs #846
Conversation
Why is this pr? |
To prune |
Please create an issue or at least describe the problem that you are trying to solve |
Behaves the same as description #848 |
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.
Looks good change!
I will compile here and see results. |
Choose #849 or this? They have the same behaviour. |
This was first, isn't it? |
Yes |
however when you |
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.
Nice enhancement, we'll port it to neo-bench.
src/Directory.Build.props
Outdated
</Project> | ||
<Import Project="$([MSBuild]::GetPathOfFileAbove('Directory.Build.props', '$(MSBuildThisFileDirectory)../'))" /> | ||
<ItemGroup> | ||
<None Include="config.json" Condition="Exists('config.json') And '$(IncludeSettingsFileOutput)' != 'False'"> |
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.
We've excluded IncludeSettingsFileOutput
from every project, so I think we don't need this condition ('$(IncludeSettingsFileOutput)' != 'False'
) to be presented here, it's just not relevant anymore.
Is config.json
a configuration file of the module's dependent library? If so, then we probably just don't need to copy it while building the module.
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.
We've excluded
IncludeSettingsFileOutput
from every project
Not every project, just part of them.
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 mean from every neo-modules project. We don't have any mentions of IncludeSettingsFileOutput=False
anymore in neo-modules.
@Ashuaidehao this breaks the publish process, please check and fix. |
* 'master' of github.com:neo-project/neo-modules: Fix workflow (neo-project#857) Fix stack exception name (neo-project#855) update workflow (neo-project#856) Refac build configs (neo-project#846) Hotfix for neo-project#850 (neo-project#853) Fix RpcNativeContract (neo-project#851)
Close #848