Add C# (.NET Core) #64

puqeko · 2020-01-23T09:54:55Z

Possible replacement for #56. However, it has been much more involved than Mono to get going.

Tested the install script on my machine and had C# compiling and running on train but a few issues came up.

The csc.dll prints everything to stdout. This means that errors don't tell nztrain that compilation failed and instead qless crashes because there is no output file. Fixed by creating a shell script to redirect the version info to stdout and the errors to stderr. The shell script needed to return the exit status of the command.
The path to the buildtime reference assemblies has a more specific version number (3.1.101) than can be specified during install (3.1.1). Hence, it is possible this path will break if someone downloads a future 3.3.1. I couldn't figure out how to make the compiler command generic in bash. Fixed thanks to @Holmes98
I'm not sure if I have included enough compile time references. It's just the one file - I tried executing a file which utilised System.Collections and it still worked so I presume all the fundamental stuff is available.
The dotnet command needed to be given a .json file to tell which runtime framework version to use.

If someone could please check over that would be great.

puqeko · 2020-01-23T22:56:54Z

Not a very descriptive commit message on my part (bac5179). I changed the shebang at some point and it turned out that isolate didn't like it so I've changed it back to \bin\bash\.

For some reason, the .sh file's output isn't being captured by isolate and I'm not sure why. The code in that area is a bit of a mess.

Consequences of not capturing output:

If the code compiles and runs successfully it will work fine.
If any errors occur then nztrain won't know and will continue until something breaks.
Version info isn't displayed.

Holmes98

~~It gives me the following error when running inside chroot (/srv/chroot/nztrain) unless /proc is mounted~~, nevermind, it works now.

/usr/bin/csc: line 10: /dev/stdout: No such file or directory
Failed to resolve full path of the current executable [/proc/self/exe]
/usr/bin/csc: line 17: /dev/stderr: No such file or directory
Failed to resolve full path of the current executable [/proc/self/exe]

script/install/debootstrap.bash

script/csharp/dotnet-config-template.json

db/languages.yml

script/install/debootstrap.bash

db/languages.yml

script/csharp/compile-command.sh

tom93 · 2020-01-29T10:34:28Z

app/models/language.rb

@@ -58,7 +58,7 @@ def self.submission_options

  # for the selection dropdown
  def self.grouped_submission_options
-    current = ["c++", "c", "python", "java", "haskell", "ruby", "j"]  # language group identifiers, order is preserved
+    current = ["c++", "csharp", "c", "python", "java", "haskell", "ruby", "j"]  # language group identifiers, order is preserved


Suggested change

current = ["c++", "csharp", "c", "python", "java", "haskell", "ruby", "j"] # language group identifiers, order is preserved

current = ["c++", "c", "python", "csharp", "java", "haskell", "ruby", "j"] # language group identifiers, order is preserved

The "Current" languages are ordered by preference; I think C# should be further down (after Python).

In that case, should we also move Python above C?

I feel like C and C++ should be together...

We should probably decide what to order on. If it is by "preference" then Python is more popular for NZIC followed by C++. Hardly anyone uses C so, although I agree that they usually belong together, it would make more sense under this scheme to have C further down.

But it seems, at least, we all agree that Python should move ahead of C#. So that's something :)

How about we merge the last two?

Favourite/Pinned/Official C++ Python Other [list everything alphabetically]

(if we can't settle on good names, we can use a separator, "----")

I think we couldn't work out how to add a non-selectable separator (without optgroups) but otherwise I like the last option the most.

Agreed. The non-selectable separator was before I started using groups, so hopefully it shouldn't be difficult.

This makes a non-selectable separator: ["────────", "", {disabled: true}]
(credit for the unicode magic: https://stackoverflow.com/questions/899148/html-select-option-separator)

tom93

Round 1 of review comments, lots of comments but they are all minor things. I've pushed a draft with the suggested changes to branch add-csharp-core-suggestions-1.

script/csharp/compile-command.sh

tom93 · 2020-01-30T06:57:04Z

script/csharp/compile-command.sh

+/usr/bin/dotnet "/usr/share/dotnet/sdk/"$VERSION"/Roslyn/bincore/csc.dll" \
+-reference:"/usr/share/dotnet/sdk/"$VERSION"/ref/netstandard.dll" "$@" 1>&2


No need for full path to dotnet

The full strings should be quoted; remove the quotes next to $VERSION

csc.dll prints version number of the C# compiler, I think the .NET Core SDK version should be added: echo ".NET Core SDK $VERSION" >&2

I would indent the continuation line 2 spaces

No need for full path to dotnet

Why then have we been specifying /usr/bin/g++ etc for the rest of them? It would be more readable to keep it short. I tested this with g++ and it worked but python did not work without /usr/bin. Perhaps further investigation in another PR.

csc.dll prints version number of the C# compiler, I think the .NET Core SDK version should be added

Makes sense. Do we bother removing the copyright line in keeping with GHC and GCC?

Why then have we been specifying /usr/bin/g++ etc for the rest of them? It would be more readable to keep it short. I tested this with g++ and it worked but python did not work without /usr/bin. Perhaps further investigation in another PR.

When a command (array of arguments) is executed using isolate, the first argument needs to be the full path to the program and the remaining arguments are passed as argv[1] etc. At first, the compilation commands were executed directly using isolate so the first argument needed to be the full path to the compiler (e.g. /usr/bin/g++).

When Java was added (32dcf29) it required two commands during compilation, javac and jar, so the compilation commands were wrapped in bash (e.g. first arg = /bin/bash, second arg = -c, third arg = javac ... && jar ...). Since the compilation command is now executed by bash, it can use shell operators like &&. It also doesn't need to use full paths to programs because bash searches for them in $PATH.

Python still requires a full path because it is an interpreter command, and they aren't wrapped in bash.

But all this is only for commands in db/languages.yml that are going to be executed directly by isolate, i.e. interpreter commands (and historically compilation commands). Inside a bash script like compile-command.sh there is no need to use full paths to programs because bash searches for them in $PATH.

csc.dll prints version number of the C# compiler, I think the .NET Core SDK version should be added

Makes sense. Do we bother removing the copyright line in keeping with GHC and GCC?

There is an option -nologo that removes the copyright message, so sure.

Ah okay. Makes sense. In which case, I'd like to propose that we remove /usr/bin from all the current compile commands to make them more readable to users. Have opened #70

script/csharp/compile-command.sh

db/languages.yml

script/csharp/dotnet-config-template.json

script/install/debootstrap.bash

tom93 · 2020-01-30T07:07:38Z

script/install/debootstrap.bash

+  cp "script/csharp/compile-command.sh" "$ISOLATE_ROOT/usr/local/csc.sh"
+  chroot "$ISOLATE_ROOT" ln "/usr/local/csc.sh" "/usr/bin/csc"


Any reason to both copy csc.sh and create a symlink? It would make sense if we were using update-alternatives to manage these links, but we're not. I would put it in /usr/local/bin, and copy it directly as /usr/local/bin/csc (the .sh suffix isn't significant).

The intension was to be consistent with the other compile commands using /usr/bin/... since that part of the command displays on the submission page. Hence, I copied it into the sandbox and symlinked them in. Could we go with update-alternatives instead?

script/install/debootstrap.bash

puqeko · 2020-01-30T09:20:24Z

Other than the few things I've pointed out, your changes seem reasonable to me. Tested and works on my install

Co-Authored-By: Holmes98 <jkhoo98@gmail.com>

…run in chroot

…tatus of the compile and run commands and send compiler output to stderr

Co-Authored-By: Holmes98 <jkhoo98@gmail.com>

tom93 · 2020-01-31T03:22:21Z

I'd prefer to use squash-and-merge because the history is complicated and not interesting. So there will be one commit with a new commit message (we can add "Co-Authored-By:" lines if we want).
If the squash-and-merge is done through GitHub, it sets the commit author to the author of the PR and the committer to "GitHub <noreply@github.com>". (Not sure what you mean by commit signature.)

puqeko · 2020-01-31T05:31:08Z

I made 6b95210 before I saw your message. Is that sort-of inline with what you were thinking? By signature I meant that it had my work email which wasn't ideal

tom93 · 2020-01-31T05:37:30Z

Yes, it's exactly what I meant.
(The squash introduced some changes, e.g. the execute permission on the scripts was lost -- probably a Windows thing -- but I'm planning to add more review comments, so we could re-squash later.)

puqeko · 2020-01-31T05:40:13Z

Okay, sounds good. Yeah I'm on a windows machine at work currently. Filling in time while stuff installs

puqeko · 2020-01-31T05:40:50Z

Should I force push?

tom93 · 2020-01-31T05:43:14Z

I'd like to keep the full history in the pull request, and only squash/force-push when merging (also there are undesirable differences in the squashed commit).

It will be required if we want to build V8 in chroot. .NET Core (#64) also requires /proc to be mounted.

tom93 · 2020-02-05T12:14:04Z

As discussed in chat, running dotnet using isolate on Linux < 4.14 produces the following error:

Failed to create CoreCLR, HRESULT: 0x8007001F

Running strace dotnet shows that it tries to use mlock, which isolate prevents:

...
membarrier(MEMBARRIER_CMD_QUERY, 0)     = 0x1 (MEMBARRIER_CMD_SHARED)
mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x2b82ffccf000
mlock(0x2b82ffccf000, 4096)             = -1 EPERM (Operation not permitted)
write(2, "Failed to create CoreCLR, HRESUL"..., 45Failed to create CoreCLR, HRESULT: 0x8007001F) = 45

It works on Linux >= 4.14 because then MEMBARRIER_CMD_PRIVATE_EXPEDITED is available, and dotnet uses that instead of mlock (see https://github.com/dotnet/runtime/issues/10568, https://github.com/dotnet/coreclr/pull/20949).

The server currently runs Linux 4.4.0, so we need to update the kernel before deploying this PR. I created issue #73 to track that.

The rest of the file doesn't use single/double quotes consistently (C/C++ have double quotes for everything; the other languages have double quotes for names and single quotes for commands). The quoting rules are different[1] but for the strings we have they don't make a difference, so let's make the new C# section internally consistent by using the same quoting style for the compiler and interpreter commands. [1] https://yaml.org/spec/1.2/spec.html#id2786942 Double-quoted strings support backslash escape sequences. Single-quoted strings don't have escape sequences, embedded single quotes must be repeated.

They got separated over time.

It's out of date (the script does more than the two things now), and at least for me the comment doesn't help much because the explanations are apparent from the script.

tom93

Round 2 of review. Draft with the suggested changes in branch add-csharp-core-suggestions-2, please review and merge into 'add-csharp-core'.

Change quoting style in languages.yml
Disable dotnet telemetry
I'm personally against telemetry, and it also raises potential issues (network access inside the sandbox, performance impact)
Remove comments in csc script (they are already out-of-date and I don't think they help much)
Warn if the kernel version is < 4.14
Move install commands to a separate file
Rename csharp directory to dotnet

script/install/csharp/dotnet-csc

It's describing the purpose of the file, not the path of an actual file.

puqeko · 2020-02-09T10:35:17Z

I haven't tested them but those changes seem reasonable to me. Making the quoting consistent for other languages could become part of PR #70 since that's related to cleaning up langauges.yml. Telemetry isn't any help to us. ~~Should we re-open this PR so that we can see it's still active under the PR tab?~~ Someone beat me to it

tom93

Other than the process limit (discussion in chat), looks good to me. I'd squash with the following commit message:

Add C# (.NET Core) (#64)

Co-Authored-By: Holmes98 <jkhoo98@gmail.com>
Co-authored-by: tom93 <tomlevy93@gmail.com>

Holmes98 · 2020-02-09T16:06:46Z

Deployed successfully.

This was referenced Jan 23, 2020

Add C# (Mono) #56

Closed

Update Haskell (using hvr's PPA) #61

Merged

tom93 self-requested a review January 23, 2020 16:58

Holmes98 reviewed Jan 23, 2020

View reviewed changes

Holmes98 reviewed Jan 24, 2020

View reviewed changes

db/languages.yml Show resolved Hide resolved

Holmes98 requested changes Jan 24, 2020

View reviewed changes

script/csharp/compile-command.sh Outdated Show resolved Hide resolved

script/csharp/compile-command.sh Outdated Show resolved Hide resolved

puqeko force-pushed the add-csharp-core branch from 60bbb91 to 2dd761e Compare January 24, 2020 03:28

Holmes98 reviewed Jan 24, 2020

View reviewed changes

script/csharp/compile-command.sh Outdated Show resolved Hide resolved

Holmes98 reviewed Jan 24, 2020

View reviewed changes

script/csharp/compile-command.sh Outdated Show resolved Hide resolved

tom93 reviewed Jan 29, 2020

View reviewed changes

Holmes98 mentioned this pull request Jan 29, 2020

Set each user's default option to their last submitted lanugage #67

Merged

tom93 requested changes Jan 30, 2020

View reviewed changes

puqeko and others added 16 commits January 31, 2020 10:37

Add install process for .Net Core

57a2b7e

Update language db and dropdown option

c989b05

Add shell scripts to simplify commands

48cc014

Update script/install/debootstrap.bash

a1f3629

Co-Authored-By: Holmes98 <jkhoo98@gmail.com>

Update script/csharp/dotnet-config-template.json

58d12b7

Co-Authored-By: Holmes98 <jkhoo98@gmail.com>

Update db/languages.yml

d005a34

Co-Authored-By: Holmes98 <jkhoo98@gmail.com>

Update script/install/debootstrap.bash

04ba568

Co-Authored-By: Holmes98 <jkhoo98@gmail.com>

Make sure things are copied to the chroot directory and commands are …

ff9746c

…run in chroot

Use file descriptors for redirect and rename to C# from CSharp

1eea31c

Update 'compile-command.sh' and 'run-command.sh' to return the exit s…

1635492

…tatus of the compile and run commands and send compiler output to stderr

Update db/languages.yml to specify .exe extensions

5b08ff4

Co-Authored-By: Holmes98 <jkhoo98@gmail.com>

Add newlines at end of .sh files

23a26fe

Make shebangs the same in both .sh csharp files

13e5f9d

Determine C# version at compilation

8d37656

Change csharp .sh to #!bin/bash so that PIPESTATUS is supported

bf54547

Add completion message to install script

6b29513

tom93 added a commit that referenced this pull request Feb 4, 2020

Only unmount /proc at the end of debootstrap.bash

e796878

It will be required if we want to build V8 in chroot. .NET Core (#64) also requires /proc to be mounted.

tom93 mentioned this pull request Feb 5, 2020

Update Linux kernel on server from 4.4.0 to 4.15.0 (HWE) #73

Closed

tom93 added 9 commits February 8, 2020 01:41

Disable dotnet telemetry

1b9c9c0

Remove a blank line in the dotnet install commands

884b2bb

Move comment summarizing the csc command to be next to the command

c237bf6

They got separated over time.

Remove comment explaining what the csc script does

6878c6c

It's out of date (the script does more than the two things now), and at least for me the comment doesn't help much because the explanations are apparent from the script.

Warn if the kernel version is < 4.14

4f91798

Move dotnet install commands to a separate file

4f90829

Rename csharp directory to dotnet

49e36d4

Merge branch 'deployed' into add-csharp-core

090228d

tom93 requested changes Feb 9, 2020

View reviewed changes

script/install/csharp/dotnet-csc Outdated Show resolved Hide resolved

Remove suffix from stdlib.dll in comment

9ee5971

It's describing the purpose of the file, not the path of an actual file.

tom93 previously approved these changes Feb 9, 2020

View reviewed changes

Use 'exec' in the dotnet-exec script to avoid creating an extra process

1d36f34

tom93 dismissed their stale review via 1d36f34 February 9, 2020 12:39

Reduce C# process limit to 5

d1bf10b

Holmes98 approved these changes Feb 9, 2020

View reviewed changes

Holmes98 merged commit 6b5b6a3 into deployed Feb 9, 2020

Holmes98 deleted the add-csharp-core branch February 9, 2020 16:06

tom93 changed the title ~~Add C# (.Net Core)~~ Add C# (.NET Core) Feb 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add C# (.NET Core) #64

Add C# (.NET Core) #64

puqeko commented Jan 23, 2020 •

edited

Loading

puqeko commented Jan 23, 2020

Holmes98 left a comment •

edited

Loading

tom93 Jan 29, 2020

Holmes98 Jan 29, 2020

tom93 Jan 29, 2020

puqeko Jan 29, 2020

puqeko Jan 29, 2020

tom93 Jan 29, 2020

tom93 Jan 29, 2020

Holmes98 Jan 29, 2020

puqeko Jan 29, 2020

tom93 Jan 29, 2020

tom93 left a comment

tom93 Jan 30, 2020

puqeko Jan 30, 2020 •

edited

Loading

tom93 Jan 30, 2020 •

edited

Loading

puqeko Jan 30, 2020 •

edited

Loading

tom93 Jan 30, 2020 •

edited

Loading

puqeko Jan 30, 2020 •

edited

Loading

puqeko commented Jan 30, 2020 •

edited

Loading

tom93 commented Jan 31, 2020 •

edited

Loading

puqeko commented Jan 31, 2020 •

edited

Loading

tom93 commented Jan 31, 2020

puqeko commented Jan 31, 2020

puqeko commented Jan 31, 2020

tom93 commented Jan 31, 2020 •

edited

Loading

tom93 commented Feb 5, 2020 •

edited

Loading

tom93 left a comment •

edited

Loading

puqeko commented Feb 9, 2020 •

edited

Loading

tom93 left a comment

Holmes98 commented Feb 9, 2020

	current = ["c++", "csharp", "c", "python", "java", "haskell", "ruby", "j"] # language group identifiers, order is preserved
	current = ["c++", "c", "python", "csharp", "java", "haskell", "ruby", "j"] # language group identifiers, order is preserved

		/usr/bin/dotnet "/usr/share/dotnet/sdk/"$VERSION"/Roslyn/bincore/csc.dll" \
		-reference:"/usr/share/dotnet/sdk/"$VERSION"/ref/netstandard.dll" "$@" 1>&2

		cp "script/csharp/compile-command.sh" "$ISOLATE_ROOT/usr/local/csc.sh"
		chroot "$ISOLATE_ROOT" ln "/usr/local/csc.sh" "/usr/bin/csc"

Add C# (.NET Core) #64

Add C# (.NET Core) #64

Conversation

puqeko commented Jan 23, 2020 • edited Loading

puqeko commented Jan 23, 2020

Holmes98 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tom93 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

puqeko Jan 30, 2020 • edited Loading

Choose a reason for hiding this comment

tom93 Jan 30, 2020 • edited Loading

Choose a reason for hiding this comment

puqeko Jan 30, 2020 • edited Loading

Choose a reason for hiding this comment

tom93 Jan 30, 2020 • edited Loading

Choose a reason for hiding this comment

puqeko Jan 30, 2020 • edited Loading

Choose a reason for hiding this comment

puqeko commented Jan 30, 2020 • edited Loading

tom93 commented Jan 31, 2020 • edited Loading

puqeko commented Jan 31, 2020 • edited Loading

tom93 commented Jan 31, 2020

puqeko commented Jan 31, 2020

puqeko commented Jan 31, 2020

tom93 commented Jan 31, 2020 • edited Loading

tom93 commented Feb 5, 2020 • edited Loading

tom93 left a comment • edited Loading

Choose a reason for hiding this comment

puqeko commented Feb 9, 2020 • edited Loading

tom93 left a comment

Choose a reason for hiding this comment

Holmes98 commented Feb 9, 2020

puqeko commented Jan 23, 2020 •

edited

Loading

Holmes98 left a comment •

edited

Loading

puqeko Jan 30, 2020 •

edited

Loading

tom93 Jan 30, 2020 •

edited

Loading

puqeko Jan 30, 2020 •

edited

Loading

tom93 Jan 30, 2020 •

edited

Loading

puqeko Jan 30, 2020 •

edited

Loading

puqeko commented Jan 30, 2020 •

edited

Loading

tom93 commented Jan 31, 2020 •

edited

Loading

puqeko commented Jan 31, 2020 •

edited

Loading

tom93 commented Jan 31, 2020 •

edited

Loading

tom93 commented Feb 5, 2020 •

edited

Loading

tom93 left a comment •

edited

Loading

puqeko commented Feb 9, 2020 •

edited

Loading