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

Cleanup capitalization and dependencies #1700

Merged
merged 12 commits into from
Nov 4, 2016
Merged

Conversation

dsyme
Copy link
Contributor

@dsyme dsyme commented Nov 3, 2016

While working through the build process I noticed a coupe of things

  • Fsi.exe should be called fsi.exe systematically. Let's normalize this now.
  • Fsi.exe is always called fsiAnyCpu.exe on Mono. Although both are odd names, it's best to use the same as what is already used on case-sensitive file systems. Let's normalize this now.
  • Fix dependencies in FSharp.Core nuget description to match project.json
  • Simplify propagation of PREFERRED_UI_LANG define

@@ -11,10 +11,9 @@
<ProjectGuid>{8b3e283d-b5fe-4055-9d80-7e3a32f3967b}</ProjectGuid>
<OutputType>Exe</OutputType>
<NoWarn>$(NoWarn);62</NoWarn>
<AssemblyName>FsiAnyCPU</AssemblyName>
<AssemblyName>fsiAnyCPU</AssemblyName>
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure this is the right capitalization?
Assembly Names guidance is Pascal casing not camel casing? Although they are in fact case insensitive in the runtime.

If you want to change the output filename on disk to camel case use the TargetFilename property.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not high priority. I'm trying to align these to those used by F# on Linux to help prevent these awkward capitalization errors in the longer term.

github.com/fsharp/fsharp has always used fsiAnyCpu. On Linux we'd expect the executable names are lowercase.exe.there is an argument it should have used fsianycpu , but I think just aligning to the existing F#-on-Linux norm is simplest.

@cartermp
Copy link
Contributor

cartermp commented Nov 3, 2016

I had experimented with removing the runtimes node before, but always got build errors. Perhaps something has changed since earlier this Summer?

Also, the RIDs in runtimes prevented me from lowering the netstandard version to netstandard1.3 (the lowest we can go due to the dependency on System.Console). Perhaps we could also lower it here? That lets it build on .NET Framework 4.6.

@dsyme
Copy link
Contributor Author

dsyme commented Nov 3, 2016

@cartermp I put the runtimes back for now. Let's do that separately. Kevin and I discussed and thought it wasn't needed, but maybe it is

@KevinRansom
Copy link
Member

KevinRansom commented Nov 3, 2016

Having thought about ... I now think they are needed and probably needed in all project.jsons. Because we shouldn’t take a reference on APIs that are not implemented in specific target platforms.
The runtimes sections will cause restore failures, if we add a dll that isn’t supported by a target platform.

@dsyme
Copy link
Contributor Author

dsyme commented Nov 3, 2016

@KevinRansom So the "runtime" section is a constraint meaning "must run on at least this platform"? Still, it seems really strange to have the names of linux distros in there.

@KevinRansom
Copy link
Member

@dsyme perhaps I i's strange ... but the coreclr does have many distro specific implementations of code. We can have a discussion over whether that is the right choice. I'm expecting we would be in agreement over that discussion and probably would wish it were different.

Kevin

@cartermp
Copy link
Contributor

cartermp commented Nov 3, 2016

Runtimes are needed for self-contained deployment of applications, but aren't intended for libraries unless you do something like take a dependency on a native binary. We shouldn't need to specify any runtime unless we've got platform-specific dependencies or native code.

I'm not sure how the move to MSBuild will affect this, though.

@KevinRansom
Copy link
Member

@cartermp Well I believe that restore fails if you have a dependency on a library that doesn't have a specific native implementation for each of the specified frameworks. So it is kind of irrelevant what the "intent" is.

@cartermp
Copy link
Contributor

cartermp commented Nov 3, 2016

So to clarify - you want to protect against the lack of a native implementation on a given target where netstandard1.6 is supposed to run, but we have some custom code in FSharp.Core that might not?

@dsyme dsyme merged commit d975693 into dotnet:master Nov 4, 2016
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.

4 participants