cc @davidfowl @lodejard
@davidfowl can you determine the design here?
Yar
Do we usually have Include suffix on that category of properties, or would it be "publish" instead if publishInclude?
If we were being consistent:
{
"compile": ["globs"],
"compileFiles": [ "file1", "file2" ],
"compileExclude": [ "globs" ],
"publish": ["globs"],
"publishFiles": [ "file1", "file2" ],
"publishExclude": [ "globs" ]
}
@DamianEdwards had a proposal to change remove the top level properties:
{
"compile": {
"include": [],
"exclude": [],
"includeFiles": []
},
"publish": {
"include": [],
"exclude": [],
"includeFiles": []
},
}
What's the difference between include
and includeFiles
? Can't it be the same property?
includeFiles don't support globs. The idea is that an IDE like visual studio can tool adding individual files by having a wide exclude globbing pattern and including individual files (for example, if you wanted to exclude an entire folder but include 2 files from it). *Files are always applied to the pattern last.
So, it's everything in include
, then filter through exclude
, and then include everything in includeFiles
?
Yep
Makes sense but it's a bit confusing because you can add individual files in include
also
Different treatment of the data. "content" globs w/out wildcard are not an error if the file is missing. And adding a rule that paths without wildcards need to be added in a different way is just an extra hoop to jump through
I'd like to see us consolidate a bunch of the top-level properties here before we ship RC2. Right now it feels somewhat hodge-podge. E.g. all options to do with compilation should be in the "compilationOptions"
property. Here's what I had in mind:
{
"version": "1.0.0-*",
"compilationOptions": {
"compiler": "csc",
"emitEntryPoint": true,
"exclude": ["wwwroot", "node_modules"]
},
"runtimeOptions": {
"gcMode": "Server"
},
"publishOptions": {
"include": [ "wwwroot", "Views", "appsettings.json" ]
}
}
@blackdwarf how do we get this looked at actively again? I'd like this done for RC2.
@DamianEdwards the whole proposal you outlined above? I will move it to RC2 and then we can triage. Sounds OK?
@blackdwarf thanks
What about testRunner? It is currently a top-level property.
Are there other properties it's logically related to? Why must it be top-level?
There is not a better node for it. At this time we don't have peer properties for it, but we may want to create a test peer to compilationOptions for future extensibility...
@DamianEdwards one thing I didn't mention, I think we closed on runtime options being in a separate conf file that is copied as content over to output.
Isn't an Options suffix on all of those properties redundant?
{
"version": "1.0.0-*",
"compilation": {
"compiler": "csc",
"emitEntryPoint": true,
"exclude": ["wwwroot", "node_modules"]
},
"publish": {
"include": [ "wwwroot", "Views", "appsettings.json" ]
}
}
at which point if you wanted to save keystrokes you could consider saing that on some nodes the "include" is the default member when the property is an array instead of an object:
{
"version": "1.0.0-*",
"compilation": {
"compiler": "csc",
"emitEntryPoint": true,
"exclude": ["wwwroot", "node_modules"]
},
"publish": [ "wwwroot", "Views", "appsettings.json" ]
}
however that second step isn't necessary if we want a single syntax
slight note on @davidfowl earlier json example - the word include probably doesn't need to be in both sets?
{
"compile": {
"include": [],
"exclude": [],
"files": []
},
"publish": {
"include": [],
"exclude": [],
"files": []
},
}
@lodejard like both your suggestions here.
@blackdwarf that's unfortunate. Details please? ASP.NET templates are going to have to have this to enable server GC.
@DamianEdwards sure, there is some discussion on dotnet/cli#757 on that. From that I remeber that we will still have the runtimeOptions (we need it for various things) but just not as part of the project.json
file. This should just mean that ASP.NET template should drop another file in there.
The latest on those issues seems to be pointing back to having these settings in project.json
which I much prefer.
@DamianEdwards hm, I was thinking on this comment. @anurse where did we land on this?
Why do you prefer project.json
for this?
@blackdwarf because each time we talk about adding files to our projects we get huge push back. Having one file for the vast majority of settings most customers will change is much easier to grok.
@DamianEdwards point taken. However, that means that I have to rebuild my app/do some transformation if I want to change some runtime options. I cannot just push new changes. It may be fine leaving this out. The approach I advocated seemed to not add huge complexity for devs and left some flexibility.
You don't have to rebuild your app to change runtime options. The compilation output includes a separate file: MyApp.config.json
with only the runtime settings that you can edit. It's just that instead of having this file in the project directory as a separate file that gets copied through, the content comes from the existing project.json and is just copied to the output in a file for use by the host (like dependencies are already)
@anurse ah, ok, so I misunderstood. Thanks for the clarification.
That proposal however, has another problem which is keeping these two files in sync. If I change the config file, but someone else changes the PJ, won't that overwrite my changes on next deployment?
Yep, but that's no different from the other proposals so far (having a config file in the project directory that gets copied to the output), and the existing configuration model (app.config
that gets copied to [MyApp].exe.config
)
@anurse yes, but it is much easier to grok if you have a separate file, is it not? It also seems a lot cleaner. You have one file which is your runtime config and that file gets copied around. Introducing this transformation step complicates things. Also, PJ data is oriented primarily towards design-time properties (e.g. compilation options, testing options, dependencies etc.).
As a note there are languages where file order matter (f# for example).
We use compileFiles
because glob compile
doesnt respect file order (or it's not guaranteed to have same order).
@blackdwarf Is it more complicated? I'm not sure I agree there. Everything else is in the project.json, why is this a separate file? I think it's more confusing that there is a separate file for this one thing. My dependencies are a runtime concept and present in the project.json. People already complain that there are too many configuration files in the default ASP.NET Core application (for RC1). This would add another one and I'm not sure it's really worth the cognitive load of explaining yet another file.
@anurse well, you are right. We are having this be a part of the project.json
now.
unassigning myself. There's a set of changes @DamianEdwards wants to make to project.json. There should be an uber issue on that.
Hey, is this planned for rc2?
cc: @DamianEdwards, @davidfowl
Yes
This is covered by the changes being made RE https://github.com/dotnet/core-proposals/blob/master/DotNetCore-ProjectModel.md in dotnet/cli#2248
@ajaybhargavb what is the ETA for this issue?
I'll send a PR by today or tomorrow.
When will the PR get merged, @ajaybhargavb
I am currently working on tests. Hopefully, I'll get this in tomorrow.
@ajaybhargavb can you rebase and send ask-mode mail to Shiproom?
44fd8bc2de3274704e16154a91e8e0222f89b435
@ajaybhargavb @piotrpMSFT
Confused ... I don't see in the commit where content
becomes publishInclude
... it looks like it says publishOptions
. What's the status on content
becoming publishInclude
? ... was that design changed?
Yes, the final decision was to change it to publishOptions
.
@ajaybhargavb ... but these are not publish "options" under the standard meaning of the word ... these files (or folders) listed in this section will _actually be published_ in the output. I thought the effort here was to align this section with publishExclude
? Why was this section named this way (if you don't mind me being perhaps overly :nose: nosy! :smile: )
Yeah... I've got to agree with @GuardRex . This makes absolutely zero sense at all.
I assume it was done to make it consistent with packOptions
and buildOptions
. But didn't want to add another layer of nesting like packOptions
has,
"packOptions": {
"files": {
"include": []
}
}
For "publishOptions" it is,
"publishOptions": {
"include": []
}
cc' @davidfowl @DamianEdwards
Edit: Updated as the comment was confusing
That's a very different kind of thing than we were understanding it would have. In that context, it does make more sense.
Agreed .... communication! If there isn't going to be an Annoucement, then we need the design notes in the issue or PR IMHO. Would save a lot of time and head-scratching.
Most helpful comment
I assume it was done to make it consistent with
packOptions
andbuildOptions
. But didn't want to add another layer of nesting likepackOptions
has,For "publishOptions" it is,
cc' @davidfowl @DamianEdwards
Edit: Updated as the comment was confusing