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

[Really Ready] dotnet sdk: disable implicitly adding system.valuetuple and fsharp.core #2528

Merged
merged 3 commits into from Aug 21, 2017

Conversation

Projects
None yet
3 participants
@0x53A
Contributor

0x53A commented Jul 15, 2017

disable implicitly adding system.valuetuple and fsharp.core
fail when a sdk f# project does not contain a reference to fsharp.core

context: Microsoft/visualfsharp#3335 (comment)

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Jul 16, 2017

Member

fail when a sdk f# project does not contain a reference to fsharp.core

I don't think we need to do that. This is basically a thing that will happen later in build. We should not have this in paket.exe itself.

Member

forki commented Jul 16, 2017

fail when a sdk f# project does not contain a reference to fsharp.core

I don't think we need to do that. This is basically a thing that will happen later in build. We should not have this in paket.exe itself.

@0x53A

This comment has been minimized.

Show comment
Hide comment
@0x53A

0x53A Jul 16, 2017

Contributor

I don't think we need to do that. This is basically a thing that will happen later in build. We should not have this in paket.exe itself

Thats the big issue - if you dont' pass any FSharp.Core to the compiler, it will silently pick any random one.

Contributor

0x53A commented Jul 16, 2017

I don't think we need to do that. This is basically a thing that will happen later in build. We should not have this in paket.exe itself

Thats the big issue - if you dont' pass any FSharp.Core to the compiler, it will silently pick any random one.

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Jul 16, 2017

Member

if you dont' pass any FSharp.Core to the compiler, it will silently pick any random one

how? I think the targets file changes forbid that!?

Member

forki commented Jul 16, 2017

if you dont' pass any FSharp.Core to the compiler, it will silently pick any random one

how? I think the targets file changes forbid that!?

@0x53A

This comment has been minimized.

Show comment
Hide comment
@0x53A

0x53A Jul 16, 2017

Contributor

Please test it, maybe i missed something, but afaik it does not error out.

Contributor

0x53A commented Jul 16, 2017

Please test it, maybe i missed something, but afaik it does not error out.

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Jul 16, 2017

Member

If you have a repro? Can you please report that to visualfsharp? It should not implicitly add things when you explicitly set the flag, right?

Member

forki commented Jul 16, 2017

If you have a repro? Can you please report that to visualfsharp? It should not implicitly add things when you explicitly set the flag, right?

@0x53A

This comment has been minimized.

Show comment
Hide comment
@0x53A

0x53A Jul 16, 2017

Contributor

Will do

Contributor

0x53A commented Jul 16, 2017

Will do

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Jul 16, 2017

Member

can you please send a second PR that only sets the properties in the targets file?

Member

forki commented Jul 16, 2017

can you please send a second PR that only sets the properties in the targets file?

@0x53A

This comment has been minimized.

Show comment
Hide comment
@0x53A

0x53A Jul 16, 2017

Contributor

I don't think that is a good idea.

When you do dotnet new and paket convert, the resulting project will be broken (and still compiling, so the user doesn't notice it).

I would say let's wait for a response on that issue, maybe there is a compilerflag to disable the magic core reference?

Contributor

0x53A commented Jul 16, 2017

I don't think that is a good idea.

When you do dotnet new and paket convert, the resulting project will be broken (and still compiling, so the user doesn't notice it).

I would say let's wait for a response on that issue, maybe there is a compilerflag to disable the magic core reference?

@0x53A

This comment has been minimized.

Show comment
Hide comment
@0x53A

0x53A Jul 16, 2017

Contributor

Great, locally I have the same three PathTooLongException failures, but I also have one that may be more interesting:

image


Edit:

Cause:

The specified SDK version [2.0.0-alpha-005165] from global.json [C:\source\Paket\global.json] not found; install specified SDK version
Did you mean to run dotnet SDK commands? Please install dotnet SDK from:
http://go.microsoft.com/fwlink/?LinkID=798306&clcid=0x409

It is picking up the global.json from the paket root folder. Probably a good idea to hardcode the cli version.

Contributor

0x53A commented Jul 16, 2017

Great, locally I have the same three PathTooLongException failures, but I also have one that may be more interesting:

image


Edit:

Cause:

The specified SDK version [2.0.0-alpha-005165] from global.json [C:\source\Paket\global.json] not found; install specified SDK version
Did you mean to run dotnet SDK commands? Please install dotnet SDK from:
http://go.microsoft.com/fwlink/?LinkID=798306&clcid=0x409

It is picking up the global.json from the paket root folder. Probably a good idea to hardcode the cli version.

@matthid

This comment has been minimized.

Show comment
Hide comment
@matthid

matthid Jul 16, 2017

Member

It feels like this test needs to be reworked. It's not working on travis either, see here

Member

matthid commented Jul 16, 2017

It feels like this test needs to be reworked. It's not working on travis either, see here

@0x53A

This comment has been minimized.

Show comment
Hide comment
@0x53A

0x53A Jul 17, 2017

Contributor

I need to test if it actually works (probably doesn't, I know myself =) ), and a few unit tests would also be nice, but from my side this is mostly finished.

As it stands, it adds FSharp.Core on convert-from-nuget and warns (I downgraded it from an error) on install if a v15 F# project does not reference FSharp.Core.

What do you think?

Contributor

0x53A commented Jul 17, 2017

I need to test if it actually works (probably doesn't, I know myself =) ), and a few unit tests would also be nice, but from my side this is mostly finished.

As it stands, it adds FSharp.Core on convert-from-nuget and warns (I downgraded it from an error) on install if a v15 F# project does not reference FSharp.Core.

What do you think?

@0x53A

This comment has been minimized.

Show comment
Hide comment
@0x53A

0x53A Aug 3, 2017

Contributor

Manual test:

  1. dotnet new lib -lang F#
  2. paket convert-from-nuget
  3. Check that both paket.dependencies and paket.references contain FSharp.Core ✔
  4. manually remove FSharp.Core from paket.references
  5. Check that a warning was printed ✔
    image

CI fails with __ PathTooLongException__ :\

Contributor

0x53A commented Aug 3, 2017

Manual test:

  1. dotnet new lib -lang F#
  2. paket convert-from-nuget
  3. Check that both paket.dependencies and paket.references contain FSharp.Core ✔
  4. manually remove FSharp.Core from paket.references
  5. Check that a warning was printed ✔
    image

CI fails with __ PathTooLongException__ :\

@0x53A 0x53A changed the title from [wip] dotnet sdk: disable implicitly adding system.valuetuple and fsharp.core to [Readz] dotnet sdk: disable implicitly adding system.valuetuple and fsharp.core Aug 13, 2017

@0x53A 0x53A changed the title from [Readz] dotnet sdk: disable implicitly adding system.valuetuple and fsharp.core to [Ready] dotnet sdk: disable implicitly adding system.valuetuple and fsharp.core Aug 13, 2017

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Aug 17, 2017

Member

what is the status?

Member

forki commented Aug 17, 2017

what is the status?

@0x53A

This comment has been minimized.

Show comment
Hide comment
@0x53A

0x53A Aug 19, 2017

Contributor

Ready from my side.

CI errors seem unrelated, but please verify.

I've rebased it on master, which wiped the CI history, so here is a link to the previous CI result: https://ci.appveyor.com/project/SteffenForkmann/paket/build/0.0.1.9005

Contributor

0x53A commented Aug 19, 2017

Ready from my side.

CI errors seem unrelated, but please verify.

I've rebased it on master, which wiped the CI history, so here is a link to the previous CI result: https://ci.appveyor.com/project/SteffenForkmann/paket/build/0.0.1.9005

@0x53A

This comment has been minimized.

Show comment
Hide comment
@0x53A

0x53A Aug 20, 2017

Contributor

AppVeyor: PathTooLongException
Travis: each stage passed at least once ;-)
image

Contributor

0x53A commented Aug 20, 2017

AppVeyor: PathTooLongException
Travis: each stage passed at least once ;-)
image

@matthid

This comment has been minimized.

Show comment
Hide comment
@matthid

matthid Aug 20, 2017

Member

Maybe we can add long-path support for the test suite on appveyor (via App.Config). We don't really care about too long paths there. As the appveyor is green on master and our only check right now I think we need it to be green here as well. Or we wait for #2584?

Member

matthid commented Aug 20, 2017

Maybe we can add long-path support for the test suite on appveyor (via App.Config). We don't really care about too long paths there. As the appveyor is green on master and our only check right now I think we need it to be green here as well. Or we wait for #2584?

@0x53A

This comment has been minimized.

Show comment
Hide comment
@0x53A

0x53A Aug 20, 2017

Contributor

Kay, let me shorten the scenario names a bit ...

Contributor

0x53A commented Aug 20, 2017

Kay, let me shorten the scenario names a bit ...

0x53A added some commits Jul 15, 2017

dotnet sdk: disable implicitly adding system.valuetuple and fsharp.core
warn when a sdk f# project does not contain a reference to fsharp.core

@0x53A 0x53A changed the title from [Ready] dotnet sdk: disable implicitly adding system.valuetuple and fsharp.core to [Really Ready] dotnet sdk: disable implicitly adding system.valuetuple and fsharp.core Aug 20, 2017

@0x53A

This comment has been minimized.

Show comment
Hide comment
@0x53A

0x53A Aug 20, 2017

Contributor

@matthid green

Contributor

0x53A commented Aug 20, 2017

@matthid green

@matthid

Looks good to me.

@0x53A

This comment has been minimized.

Show comment
Hide comment
@0x53A

0x53A Aug 20, 2017

Contributor

image

If I understand it correctly, Pack does a Restore, which goes to github, and doesn't seem to be cached.

Contributor

0x53A commented Aug 20, 2017

image

If I understand it correctly, Pack does a Restore, which goes to github, and doesn't seem to be cached.

@0x53A 0x53A closed this Aug 20, 2017

@0x53A 0x53A reopened this Aug 20, 2017

@matthid

This comment has been minimized.

Show comment
Hide comment
@matthid

matthid Aug 20, 2017

Member

Would be interesting to see WHAT is not cached. We had some problems with the bootstrapper in the past (where dotnet was calling the bootstrapper in magic mode).
Maybe we need to set some verbose flags:

  • PAKET_BOOTSTRAPPER_TRACE=true allows to set the bootstrapper into verbose mode.
  • you maybe can temporarily add "-v" to the paket targets file.

This is when this error is actually reproducible.

Member

matthid commented Aug 20, 2017

Would be interesting to see WHAT is not cached. We had some problems with the bootstrapper in the past (where dotnet was calling the bootstrapper in magic mode).
Maybe we need to set some verbose flags:

  • PAKET_BOOTSTRAPPER_TRACE=true allows to set the bootstrapper into verbose mode.
  • you maybe can temporarily add "-v" to the paket targets file.

This is when this error is actually reproducible.

@0x53A

This comment has been minimized.

Show comment
Hide comment
@0x53A

0x53A Aug 20, 2017

Contributor

no idea - I think this may not actually be an issue with paket, because the build in my fork also failed with the same error. So it seems the AppVeyor Datacenter as a whole hit the limit.

Anyway, now it is green again.

Contributor

0x53A commented Aug 20, 2017

no idea - I think this may not actually be an issue with paket, because the build in my fork also failed with the same error. So it seems the AppVeyor Datacenter as a whole hit the limit.

Anyway, now it is green again.

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Aug 21, 2017

Member

thanks!

Member

forki commented Aug 21, 2017

thanks!

@forki forki merged commit 28576a5 into fsprojects:master Aug 21, 2017

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@0x53A 0x53A deleted the 0x53A:paket-and-dotnet-and-fs branch Aug 21, 2017

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