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

FCS simplification. #773

Merged
merged 8 commits into from
Apr 29, 2015
Merged

Conversation

matthid
Copy link
Member

@matthid matthid commented Apr 27, 2015

Continuation of #771.

Hm, after fixing the Stackoverflow I noticed that we can not really forward the script output anyway (because we would loose coloring)

This PR changes the following:

  • First add a bootstrap step (which detects the stackoverflow). (build fails)
  • Fix the error
  • Change the behavior to not redirect the script output, but print the errors/warnings of FCS.
    NOTE: This is a new feature, now we can see compiler warnings from build scripts.
    Actually I was always wondering why FAKE would not show them.

@matthid
Copy link
Member Author

matthid commented Apr 27, 2015

Please tell me if I should remove the bootstrap step (commit). Added it as a verification that the error is gone (not only on my development machine).

It can probably be left there as kind of integration test.

@matthid matthid changed the title (WIP) FCS simplification. FCS simplification. Apr 27, 2015
@forki
Copy link
Member

forki commented Apr 27, 2015

Actually I like that

2015-04-27 17:50 GMT+02:00 matthid notifications@github.com:

Please tell me if I should remove the bootstrap step (commit). Added it as
a verification that the error is gone (not only on my development machine).


Reply to this email directly or view it on GitHub
#773 (comment).

@matthid
Copy link
Member Author

matthid commented Apr 27, 2015

Great to hear!
Also I just noticed this:
The bootstrap step identifies newly introduced warnings in the build script as well, see https://travis-ci.org/fsharp/FAKE/builds/60237528#L3495. Not even released and we can already see warnings in the build script :)

@forki
Copy link
Member

forki commented Apr 28, 2015

can you please fix the appveyor?

@matthid
Copy link
Member Author

matthid commented Apr 28, 2015

Thanks, didn't noticed... What's actually the problem there? looks like everything worked... I have no clue :(

@forki
Copy link
Member

forki commented Apr 28, 2015

Try to push an empty commit to rerun. Maybe it's just a hiccup.
On Apr 28, 2015 12:54 PM, "matthid" notifications@github.com wrote:

Thanks, didn't noticed... What's actually the problem there? looks
https://ci.appveyor.com/project/SteffenForkmann/fake/build/1.0.1067#L2763
like everything worked... I have no clue :(


Reply to this email directly or view it on GitHub
#773 (comment).

@matthid
Copy link
Member Author

matthid commented Apr 28, 2015

I thought that as well but all commits seem to have this. I'll try it anyway...

@matthid
Copy link
Member Author

matthid commented Apr 28, 2015

Could it be that the AppVeyor build fails when we write something to stderr?

@forki
Copy link
Member

forki commented Apr 28, 2015

yep that might be true.

2015-04-28 13:09 GMT+02:00 matthid notifications@github.com:

Could it be that the AppVeyor build fails when we write something to
stderr?


Reply to this email directly or view it on GitHub
#773 (comment).

@matthid
Copy link
Member Author

matthid commented Apr 28, 2015

That's sad, another option would be to redirect the error stream globally (and print red messages instead to stdout) or at least in TraceListener but it's quite impossible to see all the side-effects this would have...
The last change means now error messages from the FSI (build.fsx compiler errors) are printed in yellow instead of red :(. Let me first see if this fixes AppVeyor then I will look if I can get them red...

Currently we write warnings of "build.fsx" at the top, do you mind having them on the bottom?

@forki
Copy link
Member

forki commented Apr 28, 2015

Actually I don't think we should change that. Question is why do we write
to stderr but don't fail the build in FAKE
On Apr 28, 2015 1:22 PM, "matthid" notifications@github.com wrote:

That's sad, another option would be to redirect the error stream globally
(and print red messages instead to stdout) or at least in TraceListener
but it's quite impossible to see all the side-effects this would have...
The last change means now error messages from the FSI (build.fsx compiler
errors) are printed in yellow instead of red :(. Let me first see if this
fixes AppVeyor then I will look if I can get them red...

Currently we write warnings of "build.fsx" at the top, do you mind having
them on the bottom?


Reply to this email directly or view it on GitHub
#773 (comment).

@matthid
Copy link
Member Author

matthid commented Apr 28, 2015

Because FSI writes the warnings and errors to stderr... (I redirected the fsi stderr to the global stderr).
The last commit redirects fsi stderr to our stdout (including compiler errors).
This is only printing stuff, the FAKE build return status was never affected (and always correct) on any commit.

@matthid
Copy link
Member Author

matthid commented Apr 28, 2015

Ok it's still failing so AppVeyor seems to dislike something else ...

@matthid
Copy link
Member Author

matthid commented Apr 28, 2015

I think the correct fix is adding && buildServer <> AppVeyor to this line: https://github.com/fsharp/FAKE/blob/master/src/app/FakeLib/TraceListener.fs#L61.
But AppVeyor is widely used so I'm wondering why it isn't there already or maybe I'm still missing something?

@matthid
Copy link
Member Author

matthid commented Apr 28, 2015

Ok I think this is the correct thing to do and it works now.

Question is why do we write to stderr but don't fail the build in FAKE

If you want this we need to use false instead of buildServer <> AppVeyor and we would need to change https://github.com/fsharp/FAKE/blob/master/src/app/FakeLib/TraceListener.fs#L56 (so yellow lines are never printed to stderr).

Personally I feel like writing some things like warnings to stderr without failing the build is fine. (other tools use this as well to have a "nice" stdout output, that doesn't change on new versions with new warnings, like deprecation)

forki added a commit that referenced this pull request Apr 29, 2015
@forki forki merged commit efbc4bb into fsprojects:master Apr 29, 2015
@forki
Copy link
Member

forki commented Apr 29, 2015

thanks.

@forki
Copy link
Member

forki commented Apr 29, 2015

it's released. Thanks it seems to work. Good job.

@matthid
Copy link
Member Author

matthid commented Apr 29, 2015

I'm just hoping to not break everything again (Again, sorry for that). That feeling when you just broke FAKE ...

@matthid
Copy link
Member Author

matthid commented Apr 29, 2015

Good to hear that its working :)

@matthid
Copy link
Member Author

matthid commented Apr 29, 2015

Nice!

@matthid matthid deleted the simplify_fcs_interaction branch June 16, 2017 16:09
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