-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Fix cannot publish self contain twice #2467
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.
Looks functionally equivalent to before the regression, so approved. I think we have some thinking to do about this approach for other features, but we should fix the regression as you have done now.
AppHost.Create( | ||
AppHostSourcePath, | ||
ModifiedAppHostPath, | ||
AppBinaryName); |
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 forget now, is the content of AppHost a pure function of these inputs, and that's why we don't need to recreate it?
I realize this is how it was before, just in a different place. This is going to need to change when we start putting more stuff in apphost (version resources, icon, etc.)
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.
For EmbedAppNameInHost task, the old publish, it is true. only the dll name is embedded, but when dll name is changed, exe name is changed as well.
for generate shim(the only other user), I did use hash to make sure incremental build will be triggered when property changes
@wli3 was it actually introduced by #2413 ? Otherwise, we may want to consider servicing 2.1.4xx. Also, looking again at the fix, it seems that there was an intent for overwriteExisting parameter to handle this, but overwriteExisting was hidden behind a File.Exists check prior to #2413. Now it is used only for the copy, but not the subsequent SearchAndReplace. Seems that we should just delete overwriteExisting parameter and just always overwrite? Mentioning this now because I got confused reading nearby code in #2470. |
@nguerrera I am sorry. it is more like 2 commits combine. The regression could only be seen in 2.2.1xx after #2413 is checkedin. We should just always override in this case now. You are right |
This will also no longer hold true for 3.0. We will want to overwrite it because there will be user-configurable properties in their project file that affect the generated apphost. |
Good point. |
For the shims in build (tools) we optimized the incremental re-creation by hashing the settings that change the host content. We may need the same here. |
But I think removing the overwrite flag is a good start. |
Fix #2466
Introduced by this #2239
AppHost.Create will not skip when file exists. However, old the call site is not changed to encounter that. And I didn't add test coverage before changing the old code