Enhance Unreferenced Assemblies (Proposal 1)

Dec 22, 2011 at 6:08 AM
Edited Dec 23, 2011 at 6:40 AM

"Allow Specifying Unreferenced Assemblies" added the ability to add un-referenced assemblies, by explicitly adding a <references> node.

I've been working hard on a solution to interdependent NuGet building, which throws up lots of problems.

I have created a ModuleInitializer NuGet that adds a source file and build target to a project to make it easy to a ModuleInitializers to C# projects.  (I haven't added a VB version but it would be trivial, if I get the following issues resolved then I'll push it to NuGet.org).  This project includes a set of dlls in tools, a '.target' file and an install.ps1/uninstall.ps1 combo to add the target to the build.  For instrumentation NuGets this would be a very common scenario.

Issue: Firstly, empty<references/> and <dependencies/> nodes are ignored when packing from a project.  This means you must pack from a nuspec, which makes it near impossible to write automatic build scripts.  Building from a nuspec does not support automatic tag replacement (from the assembly), but most importantly it doesn't support automatic dependencies.  So in this scenario I can't find a way to prevent references being added but still have dependencies calculated from the project (without resorting to a lot of PowerShell code).

Proposal: Add the ability to explicitly exclude references/dependencies rather than just add them.

e.g. for references:

 <references>
  <reference exclude="xunit.dll" />
  <reference exclude="xunit.extensions.dll" />
</references>

& dependencies

 <dependencies>
    <dependency exclude="SparkProfiler" />
</dependencies>

 or something similar.  These exclusions will take place after references/dependencies are added by the project factory.

In general the nuspec format could do with a bit of an overhaul to enhance this combo scenario (packing from a project),  A consistent format for dependencies/references and files all the working the same way would be much cleaner.  i.e. it should work like so:

  1. If building from a project add outputs as references, add dependencies from packages, add references.
  2. If building from a NuSpec or associated NuSpec is found, add or exclude references, dependencies and files (de-duplicating additions) in node order.

File exclusion is supported in NuSpecs but not in the same way - i.e. you can add a set of files using pattern matching, but exclude some files from the pattern, this separate scenario should be clearly disambiguated in examples.

<files>
  <file exclude="bin\Debug\*.dll" /> 
  <file exclude="bin\Debug\*.pdb" /> 
  <file exclude="**\*.log" />
</files>

Although this example may be considered to 'overload' the meaning of exclude, it could be considered as an 'unbounded' exclusion.  In both cases exclude refers to the source file rather than the target file, so I believe this would be acceptable and backwards compatible.

Dec 22, 2011 at 6:30 AM

Please note that I am happy to submit a spec, implement the changes and request a pull if there's a good chance they'll be approved.

Coordinator
Dec 22, 2011 at 11:01 PM

Hmm, I think there's some incorrect data in your post.

Firstly, the <references> and <dependencies> nodes are ignored when packing from a project. 

That's not true. If you pack from a project that has a nuspec file in the same location as the project, it will merge the nuspec data into the metadata from the project. Just make sure that the nuspec file has the same name as the project.

For example, here's how I create my nuspec files after I create a project.

    nuget spec .\SomeProject.csproj

That will create a nuspec file with the correct name and the replacement values set. I just tried this out and it merged the dependencies specified in the nuspec with the ones installed in the project.

Dec 23, 2011 at 8:03 AM

The background for people who can be bothered reading long posts carefully:

Firstly, apologies that I wasn't clearer.  I have changed a couple of words in the post.  This issue wasn't that they were ignored completely (though I did say that), it was that you couldn't undo 'the magic'.  I'm fully aware that you can indeed specify a NuSpec (though see below as the reality of how it works is a lot more complex than you allude to).

From your other posts it's clear that you've designed NuGet using a 'convention over configuration' philosophy, and are determined to stick to that as much possible.  I've been using NuGet now for about 4 months in earnest and describe my scenario in the comments of an earlier post.  Following the tangents that led to I developed a PostSharp library that allowed me to scan a file structure and rebuild all the projects within it based on their dependencies to each other.

I have about 15 interdependent solutions/projects.  About 10 of them create NuGets in their own right.  By using a PowerShell script I have automated Visual Studio to scan a directory structure, calculate which projects produce which NuGets and what NuGets they depend on.  At that point I calculate which NuGets are dependent on NuGets that can be rebuilt from the structure, and then I proceed to loop through:

  1. Open project
  2. Update NuGets (inside of VS - so works properly)
  3. Rebuild
  4. Pack
  5. Publish

For each project.  In this way I can more quickly push a whole set of new interdependent NuGets far more quickly.  However, as soon as I try to do something the none-Haacky way (get it ;) ?) everything comes to a sticky hault.  So having given up trying to 'work around' the problems, I thought I'd try re-engaging here.

My fundamental issue is that I need some of the automation provided by building from a project (auto update dependencie, references, files) but I need to tweak some of those assumptions after the fact (via a NuSpec).  For example I need to add dependencies, but I don't want the files & references added automatically (because I want to move them into the tools folder and prevent them adding references).

The philosophical/dogma/heretical bit:

  1. Convention over configuration is great for end users, and it's OK for hobbyists, it's even handy for the enterprise 9 times out of 10.
  2. Convention over configuration is better when the convention is already well established and well communicated, when there's any ambiguity it can get messy fast.
  3. Convention over configuration is best when there is only one 'right way' and every other way is inferior.

As the previously referred to discussion alluded to.  Building a single NuGet is a great relatively painless task, issues only really arise when you run a big development house and have to chuck out 10s of interdependent NuGets.  In these scenarios where you're not always in control, configuration is very comforting because you can be explicit.  It can also be a lot quicker.

The compromise is Convention then Configuration.  That is, build up your best guess about what is needed by convention, but then allow everything to be overridden by the obsessive configurer (aka. The Enterprise).

To be fair, parts of NuGet allude to this approach, but with the kind of conviction that I have when I take my wife shopping.  It's often feels on these forums that you're dirty if you want to configure something, but years of developing for customers that always have a different idea of how they want to use my product than I intended (occasionally better, but let's face it normally crazy), has forced me into the conclusion that allowing them the ability to override my great idea is a real necessity.

The techy nitty gritty for those who don't have time to read the source code/logs.

NuGet can pack in two ways

  1. From a specification (NuSpec)
  2. From a project

As expected, building from a project came later (changeset 1216 went into version 1.2) than building from a specification (Convention followed Configuration in the development cycle which is normal).

You can also build a spec from a project and the pack from that.

This is great because you have the ability to point at a project and let it "do it's thing" without worrying about how, etc.  And in the majority of cases this is all you need.  Of course if there's a problem you can edit the resulting package manually using the handy editor anyway.

But sometimes you want to do a bit more, but still take advantage of the main benefit of packing from a project - the ability for the specification to stay in sync with what you actually develop.  For that reason NuGet also allows you to pack in a 3rd way

  • From a project with an associated spec.

Now firstly this is not immediately obvious if you're new to NuGet, you will possibly (like me) look for an ability to specify a NuSpec when building from a project via the command line and you won't find one so you may incorrectly assume it's not possible - but it is:

private IEnumerable<string> GetNuspecPaths()
{
    // Check for a nuspec in the project file
    yield return GetContentOrNone(file => Path.GetExtension(file).Equals(Constants.ManifestExtension, StringComparison.OrdinalIgnoreCase));
    // Check for a nuspec named after the project
    yield return Path.Combine(_project.DirectoryPath, Path.GetFileNameWithoutExtension(_project.FullPath) + Constants.ManifestExtension);
}

This bit of magic is found in ProjectFactory.cs (which is the magic engine that packs from a project), what it actually does:

  1. Look for any file in the project with a build action of content or none that ends with .nuspec and return the first one (no explicit order here so make sure you don't have 2 as there's no way of knowing for certain which one will be used).
  2. Look for a NuSpec file in the same path as the project with the same name as the project, just with the extension .nuspec instead.

This is great, because this is the Convention then Configuration approach right?  Unfortunately, not :(

If you read through the heart of the engine you discover it does this (simplified and missing steps otherwise this post would take forever):

  1. Looks for the output assembly and reads its manifest to extract the id, version, title, description and author. (from the custom attributes on the assembly, which most people stick in the AssemblyInfo.cs)
  2. Process the NuSpec and add files, dependencies and references from it, also replace tags with properties read from the assembly manifest.  There's also a bit of code in there that appears to mean if you specify an assembly author explicitly in the assembly it will take precedence over whatever you place in the NuSpec (even though there is $author$ substitution).  Maybe this was added to prevent authorship disputes, I've checked where it was added (rev. 1457) but I haven't found the justification for the special case.
  3. Finally it now adds any files, dependencies and references calculated from the project.

So it's actually a bit of a mix, it's convention-> configuration -> convention, this doesn't matter when all you can do is add stuff, but it makes overriding the convention impossible.  Specifically, excluding fiels, dependencies and references added from the project automatically in the NuSpec

The point for busy people who don't have time to read the important stuff

Alter the ProjectFactory

Change the project factory to do the following:

  1. Extract metadata from output assembly.
  2. Add calculated files, dependencies and references as per convention.
  3. Look for an associated NuSpec, and if there process it overriding anything added by convention.

I believe this would be backwards compatible as currently only addition occurs which is commutative, but obviously this may need additional careful review.

Enhance the NuSpec

  1. Add the ability to exclude files, dependencies an references.  At the moment you can add a bunch of files except those that match a pattern and this is done with an 'exclude' attribute, this should be extended so that if you don't specify the source then it will exclude all files added so far (either automatically from a project, or in the NuSpec itself by node order) that match the pattern.  References & Dependencies don't have exclusion support at all, so it's currently impossible to remove references and dependencies explicitly.
  2. Make the reference syntax identical to the files syntax for ease of understanding.  Preferably add a 'reference="true"' attribute to the files nodes to allow files to be added as references in one step.

This also can be done in a backwards compatible way.

 

I appreciate this is a long post, which is why I split it up, but I feel you missed my point completely and so I wanted to be extremely clear as to what I was actually suggesting.  I hope this allows the discussion to move forward productively.

Dec 23, 2011 at 8:21 AM

Thanks for providing more details. In addition to this, I think your point may be made clearer with some specific examples. e.g.

  • You'd like the author to come from the nuspec
  • When you use the current nuget, it doesn't allow you to do that, because x,y,z
  • With your proposal it would work because x,y,z

Just giving random example here, but you get the idea. Basically, switch the focus to very specific things that you are not able to do today (and even maybe sharing some small app that demonstrates it to avoid anbiguity), so we can think about the best way of enabling them.

Does that make sense?

Dec 23, 2011 at 11:58 AM

Sorry if it's still not clear, I am trying to communicate as effectively as possible but I think sometimes the tendency is for people to skim read, and this is perhaps a complex and subtle point.

Importantly, I never specified 'I'd like the author to come from the nuspec' I was pointing out that author is an example of ambiguous magic, it is singled out by the code for special treatment (but where is that indicated or justified in documentation).  As it works now it takes the author from the properties, if not present, already it looks it up from the output assemblies metadata (company), then it adds the author from the nuspec, then if there's more than one author it takes the first author (which would be the one specified explicitly in the command line, or the company from the metadata).  None of this is obvious without reading through a lot of code.

The argument was for a clear processing pathway that was respected consistently:

  1. Process project (if specified), taking project content. [Convention]
  2. Process project output (if available). [Convention]
  3. Process nuspec. [Configuration]
  4. Process command line. [Configuration]

I argue this is "what you'd expect" in a Convention then Configuration model.  i.e. Convention is automated and unchangeable, if it gives you what you want then fine, otherwise add a config to override it, finally if you want to do a one off change, stick it in the command line.

This should be applied to everything, id, version, title, description, author, configuration, references, dependencies and files (may have missed one or two, but I think that's everything).

So in detail:

  1. If you're packing from a project, take properties from applicable project properties.
  2. If you're packing from a project and the output is available, parse output manifest for associated properties.
  3. If there's an associated nuspec, or your packing from the nuspec, load the nuspec and substituted ${propertyName} with associated properties.
  4. If there's an associated nuspec, or your packing from a nuspec, adding, overwriting or excluding properties as specified in nuspec.
  5. Parse the command line -Prop, adding/overwriting properties.
  6. Build the package using the properties defined.

 

In terms of your second point, I did give an example and there are other examples in the discussions threads (for example content only nugets), and in fact my issue is more around the inaccessibility and inconsistency in the current 'evolved' design that I think can be cleaned up.

However let me go into more detail of a specific real world example:

  1. I have an instrumentation project that depends on Mono.Cecil, and a Utilities NuGet (that we also build).
  2. This project builds dlls that need to be added to a NuGet.
  3. I also have to add a .target file that references the built dlls.
  4. I also add an install.ps1/uninstall.ps1 to tools that add an import for the .targets file to any project that loads the NuGet.
  5. There is an example .cs.pp file that also gets added to the NuGet, and any resulting project.

This NuGet, when added to a project, adds a ModuleInitializer class, with an Initialize method, and adds a build process step to the project that modifies the assmembly adding a <Module>.cctor to the assembly which calls ModuleInitializer.Initialize() when the assembly is loaded.  (This is a really cool trick that you can't otherwise do directly in C# or VB, but you can do in .NET CIL).

The NuGet is dependant on Mono.Cecil & my Utilities NuGet.

The following issues occur:

  1. If I build from a NuSpec I can't keep the dependencies up to date.
  2. If I build from a project there is no way to prevent the dlls being added to the lib folder and references being added (so manual intervention is required)
  3. When the NuGet is added to a project unwanted references are added to the project (the project doesn't need a reference to the build task, or Mono.Cecil, etc. - the build does).
  4. When building the new project NuGet from the new .csproj, it creates a dependency to the instrumentation NuGet.  This is not desirable, as any project that includes the new NuGet will get similarly instrumented! (Which you probably don't want) - see Enhance Unreferenced Assemblies (Proposal 2).

With a cleaner process for property calculation (as suggested here) and Enhance Unreferenced Assemblies (Proposal 2), the following would be possible:

  1. Build the instrumentation NuGet from the project.
  2. Removed references using an associated NuSpec.
  3. Add a <DoesNotCreateDependency /> to the NuSpec and the resulting package, to prevent downstream NuGets automatically creating dependencies to my instrumentation NuGets (they could still add them configurably using a NuSpec if that was desirous).

 

However, this is just one example we build a lot of interdependent NuGets and now how a huge PowerShell library to work around the build process issues we're encountering with NuGets, this issue was intended to be a suggestion of a generic backwards-compatible approach to cleaning up packaging calculation that would prevent the majority of the issues that we're facing.

I would argue that trying to fix individual scenarios is not necessarily the right approach, if possible creating a more flexible process based on Convention then Configuration will allow NuGet to be used in more advanced scenarios that the core development team don't anticipate in advance.  I also believe the suggestions I've made make the process less 'magically' and more clear, allude to the spirit of what is trying to be done in the code anyway and reduce the accessibility hurdle for developers of multiple NuGets - "if it works the way you want, great; otherwise configure it".

Coordinator
Dec 23, 2011 at 6:41 PM

Thanks for the detailed response! I think my thick skull is starting to get it. :)

I know you're arguing fixing the individual scenarios is not necessarily the right approach, but we're pretty big on iterative design. Sometimes that causes pain, but often, the only way to know the solution is to solve it first and then gather feedback once you have the solution.

So let's tackle one part of the problem first. You mentioned:

Process the NuSpec and add files, dependencies and references from it, also replace tags with properties read from the assembly manifest.  There's also a bit of code in there that appears to mean if you specify an assembly author explicitly in the assembly it will take precedence over whatever you place in the NuSpec (even though there is $author$ substitution).  Maybe this was added to prevent authorship disputes, I've checked where it was added (rev. 1457) but I haven't found the justification for the special case.

That sounds like a design bug to me. I like your suggestion and it's sort of what I thought we had:

Change the project factory to do the following:

  1. Extract metadata from output assembly.
  2. Add calculated files, dependencies and references as per convention.
  3. Look for an associated NuSpec, and if there process it overriding anything added by convention.

The only thing that I'd change in your proposal here is whether the NuSpec overrides everything or whether it overrides single value elements and merges multi-value elements.

For example, would I want the <dependencies> node in the NuSpec to be merged with the dependencies added by the project or override them? Personally, I think merge is the right approach.

If you're interested in submitting a pull request that makes this consistent, we'd be happy to look at it. It'd be a good way for us to work together on something smaller and then go from here to bigger things. It's clear you've dug deeply into our code and have thought deeply about it and we definitely want to work with you to improve NuGet! Let's just take a few small steps first and then tackle the big ones. :)

I'm starting to see the value of <DoesNotCreateDependency /> but I'd rather we first solve it without a flag so that most folks never need the flag. Then in the scenarios where we absolutely do need the flag, we can consider adding it.

Also, is that the right name for the setting? I wonder if there's a higher level concept here. Is there a general type of package that should never be added as a dependency? For example, an "instrumentation" package? Etc?

Dec 26, 2011 at 7:48 AM

Hi Phil,

So for clarity by adding/overwriting/excluding with de-duplication I was defining merging not overriding, using file as an example, imagine a bin\Release folder which contains, a.dll, b.dll, c.dll, d.dll and e.dll:

Added by convention

The following files are added to the package automatically:

lib\a.dll
lib\b.dll

NuSpec configuration

 

<files>
  <file src="bin\Release\*.dll" target="lib" exclude="**\d.dll" />
  <file exclude="bin\Release\b.dll;**\d.dll" />
</files>

The first line adds  a.dll, b.dll, c.dll and e.dll.  The first two were already added by convention so they are overwritten. d.dll is not added due to the exclude pattern that applies to this line only.

The second line is the spec enhancement - exclude without a src applies to everything already added (including by convention), so this removes the b.dll added by convention and the d.dll added by the line above.  Of course this could have been written as to separate exclude lines, or as "**\b.dll;bin\Release\d.dll", etc.  Note this is a non-breaking change.

(aside: this is another area where the current specs could do with clarifying, it is not obvious whether the exclude pattern matches the src or the target - I believe from the code it's the former, but all the current examples are too ambiguous).

Result

After parsing the nuspec, the resulting package will contain:

lib\a.dll
lib\c.dll
lib\e.dll 

 

The same concept should apply to references & dependencies.  The key enhancement here is that you can now exclude files which allows the undoing of undesirable convention side effects.

I would have gone straight to a pull request but the code was in a bit of a mess (and to be fair there are almost no comments in the code - I think there appears to be a comment embargo) so I figured it might take a bit of work to refactor.  This being my first suggestion I took a look at the pull requests and found quite a lot of 'declined's, and it's quite a fundamental set of changes so I wanted to discuss before wasting a lot of time cleaning this up only to get a 'declined - we should discuss this first' response.  I will have a look now though and see about doing a pull.

Dec 26, 2011 at 8:17 AM

This is now referenced by http://nuget.codeplex.com/workitem/1808.

Dec 27, 2011 at 5:32 PM

I have pushed a mostly working version to my fork.

I've updated comments on the workitem.

I have endeavoured to minimise code changes, however the NuSpec parsing was all over the code and so I've had to do some encapsulation.  To be consistent I've moved it into a SpecFactory class (which echo's the re-worked ProjectFactory).  I've also tried to remove spec-specific code from PackageBuilder, and instead made it responsible only for building packages.

This encapsulation was genuinely necessary to allow the step by step consistency implied above.  Again I've checked in now, before adding exclusion support etc. to make as clean as possible.

There are 56 Integration tests failing which I will fix tomorrow, however the code is in a state where it makes it possible to review, so feedback would be appreciated.

Dec 28, 2011 at 4:27 PM

Submitted pull request.  All tests are working and support now added for file exclusion and dependency exclusion.

There are a number of other enhancements, e.g.

  1. Both project and nuspec file can be explicitly set from command line.
  2. Exclusions starting with "#\" will exclude based on the target rather than the source.
  3. Dependency exclusions support '*' and '?' wild cards.
  4. Authors & Owners are maintained as lists correctly and consistently.
  5. Source file spliting now supports removal of empty elements in ';' seperated lists, however an entirely empty source throws an error for backwards compatibility, and because an empty source is probably not what was intended (whereas as missing src is OK if an exclude attribute is present).
  6. Tags can be split with more characters than just ' '.

These 'enhancements' were required to allow for the encapsulation, based on the fork the pack command has encapsulated the design such that the builder is created by a series of factories, currently there are 3:

  1. SpecFactory - which builds from a nuspec, this takes all the logic that was previously split amongst the packcommand, the project factory and the builder (and a few other places).
  2. ProjectFactory - which builds from a project (by convention), this is mostly the same as the original project factory, with the NuSpec logic removed.
  3. PackCommand - which builds from pack command line properties.

The builder is now a glorified IPropertyProvider, which keeps track of build properties, which are then manipulated by factories in order.

These factories are added in an ordered list, if you specify both a spec & project the list looks like:

  1. PackCommand
  2. ProjectFactory
  3. SpecFactory
  4. PackCommand

The pack command is added at the start to ensure that command line properties are set in the builder, for use by other factories, it is added again at the end to overwrite properties set by factories with those explicitly set by the command line (so command line takes precedence).

This model makes it significantly easier to add new factories - for example adding a C++ project factory would be much easier now.

In the case of Symbol builds, the builder is created for the lib package and the factories are applied to the builder in order, and then a builder is created for the symbol package and the same factories are ran against it.  As a lot of the processing (e.g. project loading and building) can be done once in the factory, this prevents repeat building of the project (once for lib, once for symbols).

I worked hard to minimise changes and respect the OSS ethic, but there was no way to implement this discussion without some restructuring, but I tried to do so respectfully, and it's not necessarily the design I would have chosen if I had started from scratch.  Although it needs significant review, I do believe the new structure is more consistent and maintainable, and it yields more logical results.  Most importantly it provides the potential for all kinds of future enhancements.

For example, properties specified explicitly in a project are respected as 1st class citizens, and so will be used if they're not overwritten by a spec.  In theory, it would be possible (maybe not desirable) to include the nuspec in the project file itself, binding the two together more closely.  This might be a nice way forward in the case we implement a property pane on the project in VS.  The beauty is that if you then specified a nuspec it would override the properties set in the project any way.

Anyway I look forward to comments, hopefully the last 3 days won't have been a waste of time :)

Coordinator
Dec 28, 2011 at 5:23 PM

I'm looking at this right now. Hopefully one of the other NuGet devs will have some time to look at it later. There's a lot of changes in a single pull request! Ideally these would be in separate forks so we could pick and choose the ones we want, but I can understand if some of them build on other changes.

Could you post some examples of NuSpecs that are enabled by this changes? Like what the exclusions look like?

Dec 28, 2011 at 7:42 PM

AN OPEN LETTER FOR MY OPEN SOURCE FRIEND

Dear Mr Phil Haack,

I've addressed your individual fork comments against the rejected pull request.  However, being a closed pull request I figured they will probably get lost there.

I fully appreciate there were a lot of code changes (it took 3 days after all). And so the temptation was to reject on the size - ironically I think that mentality is what has caused this issue in the first place.

I read through OSS etiquette and your comments before changing a line of code and at the forefront of my work I never forgot the OSS mantra of small changes, and minimising impact, etc.

I read through the source code for the pack command in detail for a number of hours before doing anything, and I can honestly say I am now intimately familiar with the current implementation.  Further, as alluded to above, I spent several months using NuGet in earnest in various complicated (and clearly unanticipated scenarios) before settling on the idea that there may be a few improvements that would make significant differences.  The problem is that the NuGet code had (in my humble opinion) evolved over numerous versions into something of an inconsistent and unintelligible tangle.  Many of the problems I (and others) have been encountering stem from the way the code has evolved.  NuSpec parsing occurs throughout the code base in numerous classes, there is frequent code duplication (and it sometimes works differently in different locations).  There is no logical or guessable methodology save for the anticipated use cases (which are limited), depending on a combination of input factors it swaps almost randomly between looking at the command line, the spec and the project (if present).  There are numerous special case 'if ... then ... do something', and they occur all over the place.

This is not an unusual scenario, and my intent is genuinely not to criticize the work of others, and no doubt you point out the huge popularity of the project (though please don't discount the impact of MVC and Microsoft on this - though I know you have recently left them and this is a true OSS project, the adoption and usage by Microsoft through it's developer experience has had a profound impact on gold partners like my company) - rather my belief is that the current code issues are a function of the OSS methodology which shuns away from radical redesigns, instead encouraging minimalistic code changes.  At points there have to be genuine periods of consolidation activity and code cleanup.

The problem is that to allow exclusion functionality then it needs to be understood that exclusions are non-commutative, unlike the current, largely cumulative process.  When you are just adding, it doesn't matter nearly as much what order things are added in, but for exclusions order is vital.  Exclusion functionality is a core tenet of a Convention then Configuration mentality, i.e. you have to let users undo what you did for them by convention.

Before embarking on the changes I set out a design that I genuinely believe is the minimum amount of change to create a sensible and consistent additive process (it is not a throw everything away and start again design).  Only 16 source files (excluding projects, tests and 1 tools file) were changed, and only 1 new file added. I checked in this changeset yesterday evening and asked for feedback.  Unfortunately, as it's holiday season and I'm on UK time I didn't get any feedback before this morning so I had to plough on in a vacuum - I know this is no ones fault, but it is highly unfortunate.  I knew I only had today left to really focus on the changes before I'd be back into other projects.

Today I concentrated on ensuring that all the existing tests passed (which they do!) and adding the minimum exclusion functionality for files & dependencies.

Although more tests would need adding and more cleanup is possible I wanted to get it out to the community for review (and I'm glad I did before wasting further effort).

I do challenge anyone to implement a consistent clean and encapsulated packager from the existing highly un-encapsulated codebase in less code changes (it would make for an interesting exercise).

I also firmly believe that the existing structure is highly inflexible and will quickly become a maintenance impossibility that will severely hamper the project downstream, the fork proposal I made (in my humble belief) offered a genuine, albeit small, step in the direction to a more logical, maintainable and robust codebase.

I am therefore genuinely disappointed that it has been rejected for "The pull request takes on too much in one go.", as so much effort was put into doing the minimum to achieve the desired outcome.  But most disappointing is this, although I am grateful for the 9 comments you made on my fork not one referred to the overall design change or new functionality.  4 related to comments (mostly that there were some!) - which I can accept as clearly the NuGet standard is to be minimalist with commenting (which is not necessarily an encouragement to contributors), one related to the use of Code Contracts (which is really a none issue, and I didn't start it there were already ones there), one complained about a methods style that I also disliked but mirrored the existing equivalent method for files (again I was genuinely trying to respect the OSS mantra here and follow the existing code conventions), one complained at the name IFactory (when it was taken as an interface extraction from ProjectFactory) and suggested using MEF (but you rejected the code for doing too much already) one didn't understand interface inheritance and one complained that I had brought a line of code into line with the coding guidelines.

You have suggested that I "Just make the logic consistent. I think it would be best to create a new fork and stage the commits. Take on one thing at a time.", to be fair my fork does contain 13 changesets already (mostly over 2 days), and I committed at every stage I had a working build (not necessarily one that passed tests), the problem was that the existing code required a lot of consolidation to make the 'logic consistent', I can honestly say I was already obsessed with trying to make the changesets as small as possible, and I'd genuinely be interested to see how you would have done the same with less code changes.  I have strayed away from adding new functionality, except for where it was required to allow the encapsulation (for example allowing excludes to apply to targets by pre-pending a '#\' was not a throw in, but a necessary addition).  A number of enhancements came 'for free' by having a better design, rather than being implemented.

I appreciate that this may come as a whine, and after watching 3 long days of work discarded (and polishing off a bottle of wine) maybe I am not expressing myself as well I should, but I do feel the rejection comments didn't really demonstrate a genuine effort to understand what had been attempted (and I believe achieved).  I never expected the pull request to be accepted immediately, but I did hope for just a little more debate, a bit of cut and thrust, maybe even an attempt to build the code and run the tests to see if they passed.  To be fair I think I've made as much of an effort to explain and justify my contribution as can reasonably be expected (I have spent longer writing this letter than was spent rejecting my pull request), and frequently the comments I've received back have alluded at only a cursory glancing of the issues I have raised.  The problem has always been this is a subtle complex area and not susceptible to quick fixes.

I have now spent a full week (excluding Christmas day) writing, justifying, implementing and trying to make a positive contribution. I believe I have issued code changes of good quality.  This is my first real OSS foray, I run a successful development house (over 60 devs and growing) and I've been considering Open Sourcing some of our code libraries, I saw this as an excellent opportunity to get a sense of how OSS really works and to evaluate mercurial, and in that at least it hasn't been a compete waste of time so thank you, genuinely.  We're currently looking at migrating to a new hosted source control and issue tracking system and so have been looking at github/mercurial/jira/fogbugz/etc. this experience has helped inform that discussion significantly and so there are positives.  I wish I could have made a more successful contribution to the NuGet teams work, and maybe I have to accept that I'm just not enough of a team player for OSS.  After two strikes out of two I suppose I should focus on what I'm good at instead, and let my fork go the way of the many other rejected pull requests that liter the NuGet repository.

I wish you all the success in your future NuGetting, I am off to open another bottle having discovered it's Christmas.

Yours sincerely,

Craig Dean
Chief Executive
Web Applications UK
Web Applications US
Web Applications Centre of Excellence 

Dec 28, 2011 at 7:45 PM

P.S. I forgot to add that if you would furnish me with an address (does codeplex have DMs?) I will gladly send you a bottle of quality scotch in commiseration for what is undoubtedly the thankless task of maintaining an Open Source project.

Coordinator
Dec 28, 2011 at 8:08 PM

Hey Craig,

I'm sorry if this hasn't been a good experience for you. In part, I'm definitely at fault for part of that. Regarding the declined pull request, I thought that would simply send the request back to you and allow you to easily re-open it after you addressed the issues. I believe this is how the issue tracker in CodePlex works, but I can't seem to find the "re-open" button for pull requests.

We don't fully have our pull request management workflow codified. We're still working out the kinks ourselves. In fact, after I declined yours, I started  a discussion to codify the process: http://nuget.codeplex.com/discussions/284391. In doing so, I realized I closed yours prematurely.

I wanted to re-open it, but I'm unaware of how to do so. We'd still love to have your contributions, but if you don't want to continue, I understand. Please don't let this one experience sully your entire opinion of contributing to open source. 

Dec 28, 2011 at 8:15 PM

Oh don't worry, I'm surprisingly resilient to rejection, I'm well practiced :D

Now that I understand a pull rejection is supposed to mean a little better then I'd much rather try and resolve the issues than waste my work completely.

Coordinator
Dec 28, 2011 at 8:22 PM

Great! Please send a new pull request. I won't reject it off hand. I promise.  I'm not guaranteeing we'll accept the whole thing, but we can at least work through the issues. After all, I want to have one of the other devs review as well. :)

Dec 28, 2011 at 8:32 PM

I have reopened the pull request, and am now off to pretend to be not working otherwise I may be murdered by my wife.

There's a bug that means you can't hit the Save button on a comment on codeplex in Safari for iPhone ;)

Oct 10, 2012 at 7:11 PM

Craig,

I read through this entire, lengthy but, interesting thread.  I believe I am running into a similar issue (see issue) that you were experiencing.  I noticed that the file element exclude capabilities have been implemented since this thread.  I was wondering if you continued to work on the other capabilities for references and dependencies.  If so, I'd be glad to up-vote or continue with your proposed changes :)  If not I was thinking of working on them myself since according to this thread it appears that the references element does not work the way I expected.  I am still waiting to hear back on the issue I have sent in but, have begun to review the existing NuGet code to see if I can help and contribute if necessary.

-Jay