Further aspnet website build improvements#13087
Conversation
I am unsure if this has undesirable performace implications
…project depends on netstandard
…rrorstack to compiler errors and output floating properties to emitted metaproject file
| project.ProjectName); | ||
| #else | ||
|
|
||
| if (File.Exists(Path.Combine(project.AbsolutePath, "web.config"))) |
There was a problem hiding this comment.
this kind of project is compileable only on windows, right? and there the filenames are case insensitive, so there is no need to have them separate I suppose
There was a problem hiding this comment.
Good catch, I'll cleanup
| newTask.SetParameter("TargetPath", "$(" + GenerateSafePropertyName(project, "AspNetTargetPath") + ")"); | ||
| newTask.SetParameter("Force", "$(" + GenerateSafePropertyName(project, "AspNetForce") + ")"); | ||
| newTask.SetParameter("Updateable", "$(" + GenerateSafePropertyName(project, "AspNetUpdateable") + ")"); | ||
| newTask.SetParameter("Clean", "true"); |
There was a problem hiding this comment.
this does not seem right, perhaps it'd be OK to make a new AspNetClean property
There was a problem hiding this comment.
No I'm at absolutely with you here, I'm just unsure if it should be set at the solution level or as an msbuild cli argument
| && _sdkResolvedEnvironmentVariableProperties is not null | ||
| && (!_sdkResolvedEnvironmentVariableProperties.Contains(property.Name) || !String.Equals(_sdkResolvedEnvironmentVariableProperties[property.Name].EvaluatedValue, property.EvaluatedValue, StringComparison.OrdinalIgnoreCase))) | ||
| && (!Toolset.Properties.ContainsKey(property.Name) || !String.Equals(Toolset.Properties[property.Name].EvaluatedValue, property.EvaluatedValue, StringComparison.OrdinalIgnoreCase)) | ||
| && (_sdkResolvedEnvironmentVariableProperties is null | ||
| || !_sdkResolvedEnvironmentVariableProperties.Contains(property.Name) | ||
| || !String.Equals(_sdkResolvedEnvironmentVariableProperties[property.Name].EvaluatedValue, property.EvaluatedValue, StringComparison.OrdinalIgnoreCase))) |
There was a problem hiding this comment.
I got confused what happens here, but it's only a logically equivalent edit, right? (de morgan's theorem). I'd avoid changing it
There was a problem hiding this comment.
Basically when the sdk resolved environment variable is null, we don't log out any properties to metaproject files, which I feel is confusing. This basically tries to only isolate the properties that aren't explicitly set. The problem I'm having with my implementation is that metaprojects are a copy of the solution metaproject (traversal project) which sets some properties that I'm unsure if they need to be cloned
| /// <param name="commandLine">command line builder class to add arguments to</param> | ||
| protected internal override void AddCommandLineCommands(CommandLineBuilderExtension commandLine) | ||
| { | ||
| commandLine.AppendSwitch("-errorstack"); |
There was a problem hiding this comment.
I don't think this is reasonable to add for everyone. It would be confusing to users why the task is suddenly showing a lot more information. I could get behind putting it in the if (Debug){}
| newTask.SetParameter("DelaySign", "$(" + GenerateSafePropertyName(project, "AspNetDelaySign") + ")"); | ||
| newTask.SetParameter("AllowPartiallyTrustedCallers", "$(" + GenerateSafePropertyName(project, "AspNetAPTCA") + ")"); | ||
| newTask.SetParameter("FixedNames", "$(" + GenerateSafePropertyName(project, "AspNetFixedNames") + ")"); | ||
| newTask.SetParameter("Clean", "true"); |
There was a problem hiding this comment.
So it's extra clean aha (I completely didn't realise, I'll commit a cleanup in a sec)
|
After getting the netstandard issue merged the remaining thing here is -errorstack right? If you're interested in getting that in please rebase and reopen. |
2 participants
Builds off #13058 and #12980 resolving #12980, by addressing other issues I encountered when trying to build a clean version of my solution. The biggest of these is that the RAR tasks in website metaprojects don't currently detect the web.config file and by extension the assembly redirects. Also the AspNetCompiler task's doesn't report error stack traces at the moment, which I believe is an easy when to add in. I am also skipping unchanged files when copying dependencies to the Bin folder to reduce double writes. I also have changed the generated metaproject to include properties that have been set using SetProperty on the metaproject instance. I know my filtering here is not complete and my tests will fail so leaving in draft until I have that resolved. The AspNetCompiler task generated in website metaprojects also doesn't provide the clean flag at the moment. I haven't found an elegant solution to pass this in at the moment, so I have hard coded it to be true. Welcome to feedback here but I needed it to be enabled in order to avoid a cheeky null reference error. Leaving in draft for now as I await the other PR's being merged and test resolution