Skip to content

Handle dynamic OutputPath - #942

Merged
forki merged 7 commits into
fsprojects:masterfrom
allykzam:features/handle-dynamic-outputpath
Aug 23, 2015
Merged

forki merged 7 commits into
fsprojects:masterfrom
allykzam:features/handle-dynamic-outputpath

Conversation

@allykzam

Copy link
Copy Markdown
Contributor

Partial implementation so far, updates ProjectFile.GetOutputDirectory to iterate through every PropertyGroup node in the project file loading values. Need to find a nice way to process basic placeholder values (the full supported syntax is a bit expansive for my liking) and the Condition attribute. Should also provide some default values in the Map<string, string> at start, such as the buildConfiguration value.

As a side-effect of rewriting this code, I've accidentally corrected how Paket handles project files with multiple OutputPath nodes. MSBuild appears to process everything top-to-bottom, and so the last OutputPath node wins, whereas Paket will currently output a warning and use the first OutputPath it found.

allykzam added 2 commits July 25, 2015 00:06
Returns an `XmlNode seq` so that functions in the `Seq` module can be
used more easily.
Code is incomplete, as it won't process place-holder values or
conditions, but it should find and return the contents of an OutputPath
node in the project file's main PropertyGroup.
@allykzam allykzam changed the title Features/handle dynamic outputpath WIP: Features/handle dynamic outputpath Jul 25, 2015
allykzam added 3 commits July 26, 2015 13:00
Uses a regular expression to find and replace placeholder values.
Note that the matches found by the regex need to be processed in
reverse order, so that their indexes remain valid.
Rather than using hard-coded values for the OutputPath, use
placeholders, so that their handling is tested as well.

Project1.fsprojtest contains a single OutputPath node to be used for
all build configurations, and should succeed so long as the placeholder
replacement is working correctly.

Project2.fsprojtest uses a hard-coded OutputPath node for all build
configurations, and keeps hard-coded OutputPath nodes for each
individual build configuration. This should succeed if the node
condition checking is working correctly.

Project3.fsprojtest keeps the traditional hard-coded OutputPath nodes
for each individual build configuration, and should succeed if the node
condition checking is working correctly.
@allykzam

Copy link
Copy Markdown
Contributor Author

Still trying to figure out a way to handle conditions. I've got some code written using String.Split creatively that seems to handle simple conditions like '$(Configuration)|$(Platform)' == 'Debug|AnyCPU', but doesn't handle any of the condition syntax beyond == and !=. If I skip all xml nodes with conditions that this can't process, that should still be enough to handle the project I opened #914 for.

Wanting to support a little more of the condition syntax for more complex conditions, all I've come up with so far is a rather complicated regular expression. Anyone else have any thoughts?

cc/ @forki: I did get some work done on this :)

@forki

forki commented Jul 29, 2015

Copy link
Copy Markdown
Member

nice.

did you already look at the red builds?

@allykzam

Copy link
Copy Markdown
Contributor Author

Yeah. I've gotten either the Release or Debug tests to pass by making changes to the fsprojtest files, but I can't get both to pass as-is until I put something in that handles conditions. Last time I tried it at home, it was throwing an exception trying to handle line 16 of Project1.fsprojtest because I hadn't added handling for Or in conditions.

Does a really big regular expression sound like a horrible idea to you? I'll use sprintf or String.Format to build it out of smaller ones, so there isn't a 300-character regex to deal with, and a recursive function should be able to handle processing the match data it returns.

@forki

forki commented Jul 31, 2015

Copy link
Copy Markdown
Member

Does a really big regular expression sound like a horrible idea to you?

yes ;-)

@allykzam

allykzam commented Aug 9, 2015

Copy link
Copy Markdown
Contributor Author

Sorry for the delay; I kept working at the regex idea, and found that as soon as I add three or more conditions separated by and or Or, the grouping data stops being as useful. The code isn't as short, but I've started building a parser at the same gist, in case you're interested. I'd love to just throw FParsec at this, but I don't think it'd be good to add it as a dependency to Paket.

@forki

forki commented Aug 10, 2015

Copy link
Copy Markdown
Member

we had the idea to use FParsec for all parsers. but at the moment there is nobody who wants to build these.

Currently does NOT handle the `Exists` or `HasTrailingSlash` features,
nor grouping of conditions with parens. The MSDN article on MSBuild
conditions also makes mention of hex literals, which have not been
handled here either. See the following page for more information on these
features:
https://msdn.microsoft.com/en-us/library/7szfhaft.aspx

Line 737 can be changed to `doComp left right` to support performing
greater-than and lesser-than style comparisons on strings, but this is
not supported by MSBuild.
The provided configuration needs to exist in the default placeholder
data, so that if it's provided, the value isn't overridden. If the value
is empty, project files typically contain something like this to provide
a default value:

<Configuration Condition=" '$(Configuration)' == '' ">Debug</Configuration>

With this change, all tests appear to pass again, should be the last of the
changes needed for #914.
@allykzam

Copy link
Copy Markdown
Contributor Author

Finished working on the code from the gist and built it in, tests are happy now. Is there anything else I should be doing with this before it'll be acceptable for merging?

@allykzam allykzam changed the title WIP: Features/handle dynamic outputpath Handle dynamic OutputPath Aug 20, 2015
forki added a commit that referenced this pull request Aug 23, 2015
@forki
forki merged commit 8966fad into fsprojects:master Aug 23, 2015
@forki

forki commented Aug 23, 2015

Copy link
Copy Markdown
Member

this looks cool. let's see if it works in the wild wild real world ;-)

@forki

forki commented Aug 23, 2015

Copy link
Copy Markdown
Member

real

@allykzam
allykzam deleted the features/handle-dynamic-outputpath branch August 23, 2015 17:31
@allykzam

Copy link
Copy Markdown
Contributor Author

Thanks! I'll change one of my work projects back and give it a go tomorrow morning :)

@allykzam

Copy link
Copy Markdown
Contributor Author

Looks good from my FAKE script :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants