Skip to content
This repository has been archived by the owner on Dec 18, 2017. It is now read-only.

Use Release configuration as default for 'dnu pack' #3204

Closed
wants to merge 1 commit into from

Conversation

khellang
Copy link

I think this is a better default for dnu pack, maybe even for dnu publish as well?

@dnfclas
Copy link

dnfclas commented Nov 25, 2015

Hi @khellang, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by .NET Foundation and real humans are currently evaluating your PR.

TTYL, DNFBOT;

@henkmollema
Copy link
Contributor

👍, --configuration Release is too verbose.

@khellang khellang force-pushed the pack-default-release branch 2 times, most recently from c3c7cf5 to 78d4d23 Compare November 25, 2015 13:25
@khellang
Copy link
Author

OK, I have no idea how to run the tests locally, and using the CI build to run them makes the feedback loop extremely painful. Anyone seen this error before?

System.MissingMethodException : Constructor on type 'Microsoft.Dnx.Tooling.FunctionalTests.Old.DnuRestoreTests' not found.

All tests fail with the same error:

Microsoft.Dnx.Tooling.FunctionalTests Total: 320, Errors: 0, Failed: 318, Skipped: 2, Time: 0,105s

@khellang
Copy link
Author

Nevermind, hadn't set up the dev runtimes. Found a tiny message buried in a load of test failure stuff. I'll fix the tests ASAP.

@davidfowl
Copy link
Member

I disagree with this change. Is it the default for compile too? So that things are undebuggable?

Also side note, changes like this should be contributed to the new dotnet cli. We're not going to be adding new features in dnu/DNX unless they are critical bug fixes.

@khellang
Copy link
Author

@davidfowl The BuildManager picks Debug if no explicit configurations has been specified (see here). This commit passes Release to BuildManager for the dnu pack command if no configuration has been specified.

Is it the default for compile too? So that things are undebuggable?

I'm not sure what you mean? This only affects dnu pack.

Are you usually packing NuGet packages so you can debug them? Or ship debug builds in your packages? Wouldn't you consider that being the exception, rather than the rule?

You can still package debug builds, if you want - you just have to be explicit; dnu pack --configuration Debug.

Anyway, I guess this is moot, since you're killing off dnu. Let's hope the dotnet equivalent has better defaults.

@khellang khellang closed this Nov 25, 2015
@khellang khellang deleted the pack-default-release branch November 25, 2015 16:25
@khellang
Copy link
Author

BTW, @davidfowl, are you saying that dnu will be gone before RTM?

@mgravell
Copy link

Just to opine as an independent party: when it comes to a tool that is packaging things in a deployment container, defaulting to "Release" absolutely (to me) feels the natural and most expected behavior, and the one that you will almost always want. I said so much here. The one saving grace of the current process is that it does at least end up in a /debug/ folder. But I can imagine virtually everyone doing:

  • dnu pack
  • start .
  • looks in explorer
  • "wtf? why did that build debug? why would I want a debug nupkg?"
  • dnu pack --help
  • dnu pack --configuration Release

This also fortunately gets saved by the fact that there isn't currently a dnu push (to nuget), because if that also defaulted to debug, a lot of packages would be uploaded to nuget in debug mode without the author either intending it, or knowing it.

@mgravell
Copy link

If nothing else, could we perhaps have a -c === --configuration abbreviation?

@davidfowl
Copy link
Member

-c totally doable. What is said is still the case tho. We're in the process of moving everything to the dotnet cli.

BTW, @davidfowl, are you saying that dnu will be gone before RTM?

Yes

@mgravell
Copy link

Looking forward to the new tools - another excuse to blog! but ultimately, the specific tool isn't the issue here; presumably (at least in the discussion, if not the pull request) we're implicitly also talking about the equivalent functionality in the new cli.

If you're on the verge of nuking the existing tool: agree it makes absolutely no sense to worry about the PR; but the discussion ... possibly does have some life left in it.

@davidfowl
Copy link
Member

Agree. I still disagree that it should be release by default 😀 (Unless we decided that we do that for everything)

@mgravell
Copy link

@davidfowl would that actually be a terrible default? Most people debugging will be using multiple projects in an IDE, just stepping through. The IDE already specifies this explicitly and obviously...

@NickCraver
Copy link

+1 on release here - the majority use case will certainly be shipping things, which is rarely in debug. Why not default to what is, unless I'm missing something crazy, the 99% case? I'm all for including PDBs, etc. but a release build makes so much more sense here.

Take a look at how web.config behaves today on the runtime-compiled bits, debug attribute not present? Not debug. There are places where this makes sense and precedence for doing so. I think not doing this will end up with a lot of debug packages running in production and people questioning performance. The tooling is lightweight and that's awesome, but to keep the tooling and options concise they should be paired with vast majority defaults, or demand the option be specified.

@davidfowl
Copy link
Member

Sorry -1 on release. I run dnu build/pack/run way more often than I produce release builds on dnu nuget. It's a developer tool. It's the reason why you have to urn on optimization when running csc (not off). Web.config is a weird comparison, it doesn't quite map to how a tool like dnu should work

@khellang
Copy link
Author

@davidfowl We're not talking about dnu build and dnu run, those are fine as-is - we're talking about dnu pack and possibly dnu publish; how often do you run dnu pack a day and why would you want the assemblies to be non-optimized?

@akoeplinger
Copy link
Contributor

@davidfowl I agree with the other folks here, pack should default to release. Are you concerned about people being confused when build defaults to debug and pack doesn't?

@khellang
Copy link
Author

Hey, I'm all for consistency, but I don't think we should be consistent just for consistency's sake, when it makes no sense.

99% of devs will expect their package to be built using Release. It could also be a good idea to explicitly state which configuration is used when doing the actual packing (not sure if that's the case currently)?

I've been bitten by the exact scenario @mgravell described above more than once.

@henkmollema
Copy link
Contributor

Doesn't VS tooling publishes using Release by default too?

@NickCraver
Copy link

I'm not indifferent to the odd case (e.g. @davidfowl and many times myself) that wants debug by default (I consider few of us to fall in that category), but does that outweigh many not-optimized packages out in the wild that this will surely cause?

I think if there's a hard line and the .Net team isn't willing to make release the default, then there should be no default. Simply make it a required parameter. My concerns aren't with the default directly, it's the unknown number of unintentional debug builds in production it will cause.

@davidfowl
Copy link
Member

build is the same as pack. Debug should be the default and already is everywhere. If you don't want to publish debug builds then pass the release flag. It's no different to building assemblies.

@khellang
Copy link
Author

build is the same as pack

Then why is there a separate command!? 😕 This is getting ridiculous sigh 😞

Debug should be the default and already is everywhere

Why? Can you give one good reason why? I hear you saying this, but you've yet to come up with a reason, other than "I don't like it".

If you don't want to publish debug builds then pass the release flag.

But when you expect it to build release in the first place, why would you even think about passing a flag?

@davidfowl
Copy link
Member

By the same I mean it should act the same. I can't imagine having build and pack set different configurations (or even run for that matter). Debug is by far the most sensible default and has been for a while now. Like I said before, you build/run/pack locally more often than you publish to nuget.org.

We'll make it -c Release so you don't have to type out the --configuration

@khellang
Copy link
Author

Alright. I give up 😢

@tugberkugurlu
Copy link
Contributor

I prefer Debug as default for pack/build/publish commands. Reason is that I will type those commands through command line when I want to produce the output locally which is for dev/test reasons. I will type --configuration Release once when I am writing the script that will run on CI Server.

@davidfowl
Copy link
Member

I think it comes down to what you identify a nuget package with. It seems like everyone is assuming that nuget package means that I'm going to publish to nuget.org. But in this world, nuget packages are the unit of reference so it totally makes sense to have debuggable packages.

When you're done with a release and you produce the final bits, then you can optimize the hell out of it (like you do today with assemblies).

@dahlbyk
Copy link

dahlbyk commented Nov 26, 2015

But in this world, nuget packages are the unit of reference so it totally makes sense to have debuggable packages.

I think I'm missing something here... What part of a standard local web dev workflow would use pack or publish? I wonder if the sensibility of this default depends on a framework/library lens vs actual web app work?

@davidfowl
Copy link
Member

@dahlbyk Go to any of the repositories under the ASP.NET org and run build install. It'll put the locally built packages into your package folder for consumption and testing. This is one of the workflows our devs use to test changes that affect multiple repositories.

@dahlbyk
Copy link

dahlbyk commented Nov 27, 2015

Right - as framework devs your team has specific needs that seem like they might not align with what makes sense for devs building web apps with your framework. That's not to diminish your team's (and OSS contributors') needs—you're using this stuff more than anyone else pre-release—but IMO the focus needs to be on what "normal" people will do while building a web app in this new world.

I can't think of a "normal" use case to debug packed or published assets, but I may be overlooking something.

Could project.json allow specifying the default configuration to use per cli option, with scaffolding providing reasonable defaults? What happens if someone doesn't have a Debug target defined?

@benaadams
Copy link

@dahlbyk you probably want to take this to https://github.com/dotnet/cli

@davidfowl
Copy link
Member

you're using this stuff more than anyone else pre-release

I think that's the point. It's a new workflow that needed to be invented because we redefined how references work at a fundamental level. When you take that into account, more things become obvious. You basically need packages to support whatever you did when you were using assemblies. Features like this fall out of that naturally.

but IMO the focus needs to be on what "normal" people will do while building a web app in this new world.

I disagree. This command is for framework/library authors. Publishing has nothing to do with packing. They are distinct commands and have different use cases.

Could project.json allow specifying the default configuration to use per cli option, with scaffolding providing reasonable defaults? What happens if someone doesn't have a Debug target defined?

Possibly. Debug and Release are always defined and always have defaults.

@khellang
Copy link
Author

They are distinct commands and have different use cases.

I think this is the whole point of this thread; they're different and should behave different. There's no need for consistency here.

@khellang
Copy link
Author

@davidfowl Would you mind explaining what you use dnu pack for on a daily basis, and what it's supposed to be used for by library/framework authors? Is it not to create packages to push to NuGet etc.?

@davidfowl
Copy link
Member

@khellang I thought I did but I'll try again. dnu pack is used in place of compile. It's used when you want to produce a package that needs to be consumed somewhere else. I don't push the ASP.NET packages to nuget that org, our CI produces the final builds, not dev machines. After running dnu pack and getting a nupkg, I can run dnu packages add {nupkg} (that's what build install does for all things built) and that will make my locally built package visible to other projects and solutions.

Lets say I make a change to Hosting and want to consume it in MVC. You can make the change, build the hosting nupkg, run packages add, then run restore on mvc and now I'm consuming the package I just built (and it's debuggable).

Today, developers that make nuget packages that they push to nuget.org have to do this:

msbuild MySolution.sln /p:Configuration=Release
nuget pack SomeNuspec.nuspec

It's even easier now

dnu pack -c Release

@NickCraver
Copy link

@davidfowl I think it'd be helpful to illustrate how this happens, in commands:

Lets say I make a change to Hosting and want to consume it in MVC. You can make the change, build the hosting nupkg, run packages add, then run restore on mvc and now I'm consuming the package I just built (and it's debuggable).

The most pervasive problem with this movement for many (including myself) has been the lack of examples, only explanations in abstract concepts or short english exist. Most of the examples that exist for many things are so trivially short as to have very limited usefulness. IMO, that's what needs to change to get more people onboard and clear on how to do things.

@davidfowl
Copy link
Member

I just gave a very specific examples with very specific commands (twice in this thread).

Go to any of the repositories under the ASP.NET org and run build install.

CMD

build install

Poweshell

.\build install

If you're not using one of our repositories then you can manually go to a project.json and type:

MyProject/project.json

dnu pack
dnu packages add bin/Debug/MyProject.1.0.0.nupkg

Now go to another project and consume MyProject.nupkg the way you normally would, by specifying a dependency on that package.

{
   "dependencies": {
        "MyProject": "1.0.0-*" 
    },
    "frameworks": { 
         "dnxcore50": { }
    }
}

Bonus points if you change the version on every build (that should be built in)

set DNX_BUILD_VERSION={randomtimestamp}
dnu pack
dnu packages add bin/Debug/MyProject.1.0.0-{randomtimestamp}.nupkg

You can go further and use this dnu packages add command to populate another feed by specifying the location:

dnu packages add bin/Debug/MyProject.1.0.0-{randomtimestamp}.nupkg {mylocalpackagesfeed}

That feed could then be configured in my nuget.config and I can start consuming these packages from other projects.

@NickCraver
Copy link

@davidfowl Now that's a good example to illustrate how debug a good default for local dev, thank you.

I think we have differing opinions on "very specific". As a specified example, earlier you had dnu packages add {nupkg}, where {nupkg} is "path to build .nupkg", not "nupkg name", as would be the case many are familiar from using nuget.org today. That's where a very concrete example helps. I mean this as constructive criticism as points that are not clear without exact examples for those new to the new tooling. the followup and concrete commands are much appreciated and I think will help clarify things.

@khellang
Copy link
Author

Having your CI server pushing to NuGet.org is one thing - I don't think it matters much in that case, since it's a machine "typing the command" anyway. But many smaller OSS projects don't necessarly have that infrastructure in place, or they're doing "trunk-based" development and don't want every commit to master resulting in a package on NuGet.org. They'd end up with the scenario that @mgravell mentioned above, at least the first couple of times, until you learn that you have to specify -c Release. As mentioned earlier, it would be nice to output to the console what configuration your packaging with.

I guess we've settled the command itself and its defaults, -c Release should be fine. That I'm lazy and don't want to type --configuration Release is not really the main issue here. My main concern (which was the reason for this proposed change) hasn't been addressed yet; packages with debug binaries ending up on NuGet.org.

In this new world, where packages are the unit of reference, it would make sense to have a way to identify "debug packages" and have the push command warn/ask you if try to push them to NuGet.org, like @haacked mentioned on Twitter yesterday. Is that something that's possible today? Or do you have to guess what kind of binaries are in a package?

@davidfowl
Copy link
Member

Having your CI server pushing to NuGet.org is one thing - I don't think it matters much in that case, since it's a machine "typing the command" anyway.

@khellang Our CI does not push to nuget.org. Our CI produces packages that a human pushed to nuget.org (I wish it were more automated though).

But many smaller OSS projects don't necessarly have that infrastructure in place, or they're doing "trunk-based" development and don't want every commit to master resulting in a package on NuGet.org.

We auto push to myget.org not nuget.org. I think it would be crazy bad to auto publish to nuget.org but I know of some projects that do this.

They'd end up with the scenario that @mgravell mentioned above, at least the first couple of times, until you learn that you have to specify -c Release.

How is that any different from doing this today:

msbuild MySolution.sln
nuget pack path/to/SomeNuspec.nuspec

Oops! That's a debug nupkg I just built. Same problem. It's just that msbuild has been around for ~15 years and dnu is a baby.

As mentioned earlier, it would be nice to output to the console what configuration your packaging with.

I could merge a PR for that 😉 . You'd want to make it for the .NET CLI as well though.

OK, I guess we've settled the command itself and its defaults, -c Release should be fine. That I'm lazy and don't want to type --configuration Release is not really the main issue here.

Yea, it should be shorter.

My main concern (which was the reason for this proposed change) hasn't been addressed yet; packages with debug binaries ending up on NuGet.org.

I actually don't worry about that so much. I'd actually prefer if both were on nuget.org. Have you tried to debug a roslyified optimized dll? It's impossible.

In this new world, where packages are the unit of reference, it would make sense to have a way to identify "debug packages" and have the push command warn/ask you if try to push them to NuGet.org, like @haacked mentioned on Twitter yesterday. Is that something that's possible today? Or do you have to guess what kind of binaries are in a package?

It would require a change to nuget push and I'm not even sure how you would detect it other than location on disk (/debug) folder.

If I had my way (and it'll happen eventually), there would be a way to push both packages to any nuget feed (we actually had a prototype of this). Feeds would then support debug and release packages by default (local and remote) and all of the tooling we have for locating symbols would be aware of this. dnu pack would probably compile for all configurations by default and nuget push would push both packages to the feed by default. At consumption time, there'd be an experience for getting the right package based on restore flag or gesture in the project file. It would also play into making packages debuggable by default (symbol packages with source).

/braindump

@benaadams
Copy link

That feed could then be configured in my nuget.config and I can start consuming these packages from other projects.

This is so much easier than what I've been doing; thank you for persisting with your explanations.

@khellang
Copy link
Author

Oops! That's a debug nupkg I just built. Same problem. It's just that msbuild has been around for ~15 years and dnu is a baby.

The difference is, as you mentioned, that msbuild already existed, and that NuGet had no control over building the binaries - it just had to accept what it received. Now these commands have been merged for specific use cases; making a single tool own the entire workflow. This means that you have more knowledge and could be "smarter" about building, packaging and publishing, IMO 😄

I could merge a PR for that . You'd want to make it for the .NET CLI as well though.

Is there any point? If you're saying it'll be removed by RTM, there won't be many releases left before it's gone. I'll definitely have it in mind for dotnet.

If I had my way (and it'll happen eventually)...

Sounds really nice 😁

@davidfowl
Copy link
Member

Nuget pack has -build option as it exist today and the same problem exists there. These aren't excuses btw I'm just stating that the same workflow does exist today and it's well understood how to handle it. Nothing has changed per se.

We could add a dnu pack --no-compile option but meh.

@haacked
Copy link

haacked commented Nov 30, 2015

I'm not even sure how you would detect it other than location on disk (/debug) folder

Ideally, dnu pack would embed that information into the nupkg when packing.

Until then, a heuristic you could look at is to check if any assembly in the lib folder has the System.Diagnostics.DebuggableAttribute set.

@khellang
Copy link
Author

Ideally, dnu pack would embed that information into the nupkg when packing.

Yes, that's what I meant. I see zip files supports an "extra" field in both local file, as well as central directory headers (see https://en.wikipedia.org/wiki/Zip_(file_format)#File_headers). This could be utilized to add extra metadata that could be read without unzipping the manifest file. Don't know if the .NET APIs support these, though.

Update: These fields are definitely read, but not exposed anywhere: https://github.com/dotnet/corefx/blob/c02d33b18398199f6acc17d375dab154e9a1df66/src/System.IO.Compression/src/System/IO/Compression/ZipArchiveEntry.cs#L40-L41

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet