Skip to content
This repository has been archived by the owner on Oct 31, 2021. It is now read-only.

upgrade to rc2, add embedded resources, fix test/dnx run/dnx web #24

Merged
merged 9 commits into from
Dec 17, 2015

Conversation

enricosada
Copy link
Contributor

fix #18 , #15, #9

  • use Microsoft.Extensions.Compilation instead of Microsoft.Dnx.Compilation

  • pin sdk version

  • pin FSharp.Dnx version to dnx 1.0.0-rc2

  • fix dnx451 deps

  • use xnunit dnx runner

  • add doc

  • add tests without FSharp.Dnx

  • log compiler arguments with debug build

  • fix resolve FSharp.Core.resources
    ResolveHooker.HandleResolve should consider only FSharp.Core, but there was
    a bug with FSharp.Core.resources with assembly name

    FSharp.Core.resources, Version=4.4.0.0, Culture=it-IT, PublicKeyToken=b03f5f7f11d50a3a

  • fix dnx run

  • add all directory with project.json to global.json

  • add support for embedded resources

  • use embedded resources instead of a separated mvc views project

  • add docs

Review on Reviewable

- use Microsoft.Extensions.Compilation instead of Microsoft.Dnx.Compilation
- pin sdk version
- pin FSharp.Dnx version to dnx 1.0.0-rc2
- fix dnx451 deps
- use xnunit dnx runner
- add doc
- add tests without FSharp.Dnx
- log compiler arguments with debug build
- fix resolve FSharp.Core.resources
  ResolveHooker.HandleResolve should consider only FSharp.Core, but there was
  a bug with FSharp.Core.resources with assembly name

  FSharp.Core.resources, Version=4.4.0.0, Culture=it-IT, PublicKeyToken=b03f5f7f11d50a3a

- fix dnx run
- add all directory with project.json to global.json
- add support for embedded resources
- use embedded resources instead of a separated mvc views project
- add docs
@Alxandr
Copy link
Contributor

Alxandr commented Dec 15, 2015

First of all. This (in general) seems great. There are a lot of great things here. Especially the fact that you've added support for embedded references. Really great stuff.

Some points:

  • master should not be bound to a specific version of version of DNX or the libraries. It should work against latest. It's built daily and published to myget (much like the unstable sources for DNX itself).
  • what's the point of FSharp.Dnx.UnitTests? It's written in C# as far as I can tell.

Reviewed 36 of 36 files at r1.
Review status: all files reviewed at latest revision, 11 unresolved discussions.


docs/development.md, line 10 [r1] (raw file):
dnvm upgrade -u


global.json, line 3 [r1] (raw file):
Neither sample nor test should be listed in projects. Projects is to enable lookup of projects based on name. We do not want src projects to be able to reference test projects for instance.

SDK version should not be locked in.


NuGet.Config, line 8 [r1] (raw file):
Don't use cidev. It's the same as aspnetvnext, just not unit-tested.


README.md, line 9 [r1] (raw file):
master is not pinned. Release commits are.


README.md, line 39 [r1] (raw file):
You don't need to build before you test. dnx test is enough.


README.md, line 48 [r1] (raw file):
Same here as with tests. No need to build first.


README.md, line 63 [r1] (raw file):
Same here, no need to build.


sample/HelloFSharp/project.json, line 7 [r1] (raw file):
Where did you get this from? Never seen it before.


sample/HelloFSharp/project.json, line 10 [r1] (raw file):
This would simply allow you to run dnx HelloFSharp instead of dnx run. What's the point of that?


src/FSharp.Dnx/FileWriteTimeCacheDependency.cs, line 7 [r1] (raw file):
Where is this used? What's it used for? I probably just missed it skimming through the code :).


src/FSharp.Dnx/project.json, line 8 [r1] (raw file):
Do not tie down to rc2


Comments from the review on Reviewable.io

@enricosada
Copy link
Contributor Author

sry @Alxandr , i wrote the comments in https://reviewable.io but now require write permission to my repo to publish it, and i dont want to do that.

As a personal note (my feedback, i dont want to change your workflow), it's nice to track pending review, but ihmo it doesnt add a lot
It has some pro, unique email for multiple notification, dashboard, for you what is the killer feature? I I am trying to evaluate the tool atm, if you have 2 min for a feedback as user 😄

Cons:

  • github remove obsolete comments if add a commit on same line ( resolved review )
  • ui has issue on mobile ( for example doesnt work right on my iphone chrome/safari )
  • less linking inside github ( comments etc )
  • pending review/status can be done with label in github

I'll write there my comments

docs/development.md, line 10 [r1](raw file):
dnvm upgrade -u

ok

global.json, line 3 [r1](raw file):
Neither sample nor test should be listed in projects. Projects is to enable lookup of projects based on name. We do not want src projects to be able to reference test projects for instance.

SDK version should not be locked in.

ok for samples/tests

global.json sdk is used by visual studio, it's the minimum required to run. this branch require rc2 at least.
I'll remove for now, and add a note in readme to manually set sdk

NuGet.Config, line 8 [r1](raw file):
Don't use cidev. It's the same as aspnetvnext, just not unit-tested.

ok, i'll remove it

README.md, line 9 [r1](raw file):
master is not pinned. Release commits are.

i'll write something like This project's branchs are expected to work with different version of aspnet/dnx

README.md, line 39 [r1](raw file):
You don't need to build before you test. dnx test is enough.
README.md, line 48 [r1](raw file):
Same here as with tests. No need to build first.
README.md, line 63 [r1](raw file):
Same here, no need to build.

i think is good to show how to build and after run.
But np, i'll remove it

sample/HelloFSharp/project.json, line 7 [r1](raw file):
Where did you get this from? Never seen it before.

new from rc1, see http://stackoverflow.com/questions/33888632/what-does-compilationoptions-emitentrypoint-mean

mean compile as console app instead of library

sample/HelloFSharp/project.json, line 10 [r1](raw file):
This would simply allow you to run dnx HelloFSharp instead of dnx run. What's the point of that?

because with this one, i can Debug -> Start in visual studio.
Without i get the following error

✂-1

Doesnt work anyway (design time something, i'll add an issue

System.NotSupportedException: Specificato metodo non supportato.
   in Microsoft.Dnx.Compilation.DesignTime.DesignTimeProjectReference.EmitAssemb
ly(String outputPath)
   in FSharp.Dnx.FSharpCompiler.CompileProject(CompilationProjectContext project
Context, IEnumerable`1 incomingReferences, IEnumerable`1 incomingSourceReference
s, Func`1 resourcesResolver) in D:\github\fsharp-dnx\src\FSharp.Dnx\FSharpCompil
er.cs:riga 109

i think DesignTimeProjectReference should be extended too, i'll check

src/FSharp.Dnx/FileWriteTimeCacheDependency.cs, line 7 [r1](raw file):
Where is this used? What's it used for? I probably just missed it skimming through the code :).

i was trying to remove Microsoft.Dnx.Runtime.Sources dependency, next pr.
i'll remove this file

src/FSharp.Dnx/project.json, line 8 [r1](raw file):
Do not tie down to rc2

the minimum version required is rc2, with rc1 doesnt work.
Should i ignore that and leave like before? I can try to leave previous version

@enricosada
Copy link
Contributor Author

btw @Alxandr you have time for a chat? slack/skype
i want to help, if you have time so i can understand better the problems, what's next todo

@enricosada
Copy link
Contributor Author

what's the point of FSharp.Dnx.UnitTests? It's written in C# as far as I can tell.

It's to check if xnunit works, and make it easier to add unit tests of c# code.
If FSharp.Dnx doesnt work, tests cannot be run, and is difficult to know why
I added two tests but i removed them from this pr, to minimize diff. This pr was already large enough for a first pr, i need to understand this repo better, and this add rc2, fix, embedded res

@enricosada
Copy link
Contributor Author

ok understood visual studio, sry i am newbie of dnx

dnvm upgrade -u

upgrade dnvm version to unstable, and change default alias globally, adding to PATH env var of user.

In Visual studio and VSCode, if the global.json sdk property is not set, the default alias is used as fallback.
But that mean we change a global config.
It's ok, but i'll document that, because if someone ( like me ) is new to dnx or this repo, get strange errors and after that, other vs solutions got issues

maybe there is a way like

dnvm install latest -a latest-unstable # with ci feed, latest should be rc2
dnvm use latest-unstable

and set unstable in global.json

just writing down ideas

@Alxandr
Copy link
Contributor

Alxandr commented Dec 16, 2015

I'm available at skype (AlxandrHeintz).

With regards to upgrade, that is indeed a combination of install, use and setting the default alias, it's just what I've always used :). The problem with not doing upgrade (afaik) is that there is no latest aliasing. At least there wasn't before. So you had to install latest, then do use <insert-long-and-painfull-version-string-here>, which was not fun. Thus upgrade was a lot easier. Still not sure I understood the point of the added unit-tests, and I can also discuss the use of reviewable, though don't think this is the right place for it. But it's provided me a good few boons, mainly in the aspect that it somewhat forces me to look through all the files that's changed, and also acknowledge any comments that are made. It doesn't allow me to oversee anything (at least not as easily).

The fact that you need to add commands is new. Also, in dnx, the reason why build before run is useless, is because if you do build than run, the built code isn't used at all. It still builds it in-memory before running (unless it's changed really recently). So the building becomes an extra action that serves virtually no purpose at all. It does not even speed up launching.

@Alxandr
Copy link
Contributor

Alxandr commented Dec 16, 2015

Oh, and the "emitEntryPoint": true should probably be handled by the FSharpCompiler (or similar), so that if you have it set to true, and no main method is provided, it fails the compilation (compilation flag that says it's an executable instead of library).

@enricosada
Copy link
Contributor Author

Thx @Alxandr i'll add you in skype

yes, emitEntrypoint is another thing i want to add, now run doesnt work in visual studio ( because of missing entry point , i need to check that too, i'll open an issue after this is merged if is correct to use it )

About install, we can use latest argument, and add -alias something

D:\github\fsharp-dnx>dnvm install latest -u -alias latest-unstable
Determining latest version
'dnx-clr-win-x86.1.0.0-rc2-16312' is already installed in C:\Users\E.SADA\.dnx\r
untimes\dnx-clr-win-x86.1.0.0-rc2-16312.
Adding C:\Users\E.SADA\.dnx\runtimes\dnx-clr-win-x86.1.0.0-rc2-16312\bin to proc
ess PATH
Setting alias 'latest-unstable' to 'dnx-clr-win-x86.1.0.0-rc2-16312'

creating a latest-stable

D:\github\fsharp-dnx>dnvm install latest -alias latest-stable
Determining latest version
'dnx-clr-win-x86.1.0.0-rc1-update1' is already installed in C:\Users\E.SADA\.dnx
\runtimes\dnx-clr-win-x86.1.0.0-rc1-update1.
Adding C:\Users\E.SADA\.dnx\runtimes\dnx-clr-win-x86.1.0.0-rc1-update1\bin to pr
ocess PATH
Setting alias 'latest-stable' to 'dnx-clr-win-x86.1.0.0-rc1-update1'

@enricosada
Copy link
Contributor Author

Sry if i continue this, it's only a feedback as new user, i got errors in visual studio because default alias was rc1 ( this branch require rc2 ) and vice versa.
So i was trying to find a way to have a clone -> run experience without change default alias ( because that mess global config, and if someone like me is new to dnvm, is a issue ).
I'll add documentation to upgrade -u i think is enough

@enricosada
Copy link
Contributor Author

ok done

@Alxandr
Copy link
Contributor

Alxandr commented Dec 17, 2015

Reviewed 9 of 9 files at r2.
Review status: all files reviewed at latest revision, 2 unresolved discussions.


Comments from the review on Reviewable.io

Alxandr added a commit that referenced this pull request Dec 17, 2015
upgrade to rc2, add embedded resources, fix test/dnx run/dnx web
@Alxandr Alxandr merged commit b22bde6 into fsprojects-archive:master Dec 17, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Build broke
2 participants