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

Build system cleanup #713

Closed
wants to merge 12 commits into from
Closed

Build system cleanup #713

wants to merge 12 commits into from

Conversation

SamB
Copy link
Contributor

@SamB SamB commented Nov 5, 2015

I'm trying to clean up some things about the build system, among them the near-duplicate -proto projects (#712), but I don't actually want my changes pulled yet; I'm only creating this pull request in order to [ab]use the CI infrastructure.

Edit: It seems like this might be a good time to merge what I've done so far, as you seem to like it and it might take me a while to script the generation of the -proto projects.

For greppability, those files are:
    FSharpSource.targets
    FSharpSource.Settings.targets

I'm doing this because I had trouble remembering which file gets included
at which end of a project file, which is pretty important to know when
thinking about whether you can use such-and-such property in either a
project file or the other .targets file, or which file a given property
definition needs to appear in.
@msftclas
Copy link

msftclas commented Nov 5, 2015

Hi @SamB, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla.microsoft.com.

TTYL, MSBOT;

@msftclas
Copy link

msftclas commented Nov 5, 2015

@SamB, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, MSBOT;

Since I plan to do away with the -proto proto projects in favor of the
unsuffixed projects built with Configuration=Proto, it seems wise to
add validation that the project in question actually supports this.
This only affects references to non-F#-related libraries.

The point of this commit is to verify that there's no particular
reason for these references to be different in proto builds.  If
there is, then the resulting "git bisect" should finger this commit.
Visual Studio already knows about these files from their CustomCopyLocal
items, so having Content items for them too can only serve to confuse.
@SamB
Copy link
Contributor Author

SamB commented Nov 6, 2015

By the way, I've been using this little shell script to compare the -proto projects with the normal versions;

#!/bin/sh
for i in FSharp.Build FSharp.Compiler Fsc; do
    git --no-pager diff --no-index src/fsharp/{$i/$i,$i-proto/$i-proto}.fsproj;
done

(Don't forget chmod +x; I was a little surprised that this was needed on Windows since I thought most files had execute permissions here, but for some reason I had to do that before bash autocompletion worked.)

Also, my laptop power cord is busted, plus I'm going to be visiting with a friend I haven't seen in a while, so I may be out of touch for at least the next couple of days.

@KevinRansom
Copy link
Member

Wow!!! thanks for diving into the build, you are surely a brave person. We need all of the help we can get with making it a bit less opaque.

This was mostly done by copying and pasting the items from the normal
projects to the -proto projects, then commenting some .fs items out in
FSharp.Compilers-proto.fsproj (at least one of which broke the build until
I commented it out).  (Also fixing up a few relative paths, naturally.)
@SamB
Copy link
Contributor Author

SamB commented Nov 10, 2015

Oh dear, some sort of phase-crossing seems to have crept in somehow :-(

Edit: Looks like I still didn't manage to actually pass the correct configuration down to the MSBuild task from fsharp-proto-build.proj.

@@ -456,6 +462,21 @@
<Compile Include="..\fsc.fs">
<Link>Driver\fsc.fs</Link>
</Compile>
<!--
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this comment - it's only understandable in the context of the specific "no duplication" aims being discussed in this PR. thanks

@dsyme
Copy link
Contributor

dsyme commented Nov 10, 2015

I'm generally ok with most of the PR roughly up to @1571d5c and greatly appreciate it. Up to there the PR achieves good textual alignment between matching Proto and non-Proto build files which is a goal I support. To me that goal is enough for now please.

Building the proto from the non-proto files is not an outcome I particularly care for. We used to have it like that years ago it was a PITA. Specifically, it means you need to pass a configuration parameter to get the proto build - this is irritating and error prone, and trying to achieve this kidn of sharing always seems to confuse tools like Visual Studio and Xamarin Studio (let alone the F# support in Atom, Emacs, ...)

So please I'd ask you to just keep two duplicated (but textually aligned) project files - we've had it that way for ages now and it's pretty clear what's going on.

thanks
don

@SamB
Copy link
Contributor Author

SamB commented Nov 11, 2015

@dsyme: Hmm. That would also save me from having to figure out WTH the Configuration isn't getting set the way I'd expect in the final projects, which I was thinking might require me to add more diagnostics to MSBuild.

But if doing away with the -proto projects isn't an option (and turning them into thin wrappers around the normal projects is also out), then I guess I'd want to generate them as part of the build, so they're forced to be in sync with the main ones. (And that it might make the logistics easier to not do it using F# code; C# would likely be simplest, or possibly VB?)

@enricosada
Copy link
Contributor

then I guess I'd want to generate them as part of the build, so they're forced to be in sync with the main ones

@SamB dont add anoter pre build step pls. ideal workflow is:

  • clone repo
  • open solution
  • run

there are already lots of prebuild step requirements, and build infrastructure is already weak (.bat files, multiple steps)

Maybe add a command ( .fsx? ) to generate it, so we can use it and commit generated/updated -proto proj to repo.
if we need to check sync, we can add a test to check if generated output is the same of actual file in the repo
File list does not change alot, no need to recreate the -proto proj before each build

@dsyme
Copy link
Contributor

dsyme commented Nov 11, 2015

@SamB Yes, I think the ideal for now is to still have the two project files, just have them much better aligned.

@enricosada
Copy link
Contributor

btw @SamB thx for your work, the infrastructure/build system need some love

@SamB
Copy link
Contributor Author

SamB commented Nov 11, 2015

@enricosada Hmm, yes, a manually-run script with an assert somewhere to verify freshness would certainly work, given that the motivation of having it build every time would only be to ensure that it could not get out of sync. (I believe I had momentarily forgotten about the fact that .sln files are not a kind of MSBuild file, which would have made it reasonably easy to arrange that the project files be automatically regenerated.)

And I've looked at trying to reduce the reliance on batch scripting in the build; in fact, I have an MSBuild .proj stashed somewhere that's meant to distill each batch of msbuild invocations in section 3 of DEVGUIDE down to just one invocation, and it seemed to work for me. It sounds like you might appreciate it if I would polish that off and submit it sooner rather than later?

@enricosada
Copy link
Contributor

@SamB yes, that part is really annoying, i usually do appveyor.bat and a custom script but is not a solution for new contributors.
Without changing the build system, helper commands (like your msbuild proj to run bat) may help.
Or another bat file, sigh

@SamB
Copy link
Contributor Author

SamB commented Nov 11, 2015

@enricosada: Actually, it doesn't run bat, it does recursive MSBuild on all the relevant projects, including FSharp.Core and its tests for all of the different target frameworks that DEVGUIDE lists. One of the nice things about this is that it lets MSBuild summarize all of the errors at the end.

@SamB SamB changed the title [DO NOT MERGE] Build system cleanup Build system cleanup Nov 11, 2015
@enricosada
Copy link
Contributor

a lot better if summarize all errors 😄 ( + stop at first failing project if possible )

*Except it actually doesn't do the proto build at the moment.**

**Or the VS integration.
We didn't need that half, honest!
Here we get to replace that huge batch of msbuild invocations not just
once, but twice, since it appeared in both Debug and Release variants.
@SamB
Copy link
Contributor Author

SamB commented Nov 14, 2015

@enricosada: So, kind of like this?

@SamB
Copy link
Contributor Author

SamB commented Nov 14, 2015

@dsyme So is there anything else that I need to do before this bit is merged?

@KevinRansom
Copy link
Member

The requirements for cross platform will require a different approach.

Closing for now.

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.

None yet

5 participants