Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle dynamic OutputPath #942

Merged
merged 7 commits into from
Aug 23, 2015
Merged

Handle dynamic OutputPath #942

merged 7 commits into from
Aug 23, 2015

Conversation

allykzam
Copy link
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
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
Copy link
Member

forki commented Jul 29, 2015

nice.

did you already look at the red builds?

@allykzam
Copy link
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
Copy link
Member

forki commented Jul 31, 2015

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

yes ;-)

@allykzam
Copy link
Contributor Author

allykzam commented Aug 9, 2015

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
Copy link
Member

forki commented Aug 10, 2015

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
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
Copy link
Member

forki commented Aug 23, 2015

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

@forki
Copy link
Member

forki commented Aug 23, 2015

real

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

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

@allykzam
Copy link
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