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

Various enhancements for dotnet #4362

Merged
merged 5 commits into from
Dec 27, 2022

Conversation

hez2010
Copy link
Contributor

@hez2010 hez2010 commented Nov 29, 2022

  • Split execution and compilation to avoid unnecessary crossgen2 run
  • Add nuget packages support

Copy link
Member

@RubenRBS RubenRBS left a comment

Choose a reason for hiding this comment

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

Changes look good, thanks!

etc/config/csharp.amazon.properties Outdated Show resolved Hide resolved
@RubenRBS
Copy link
Member

Hi! Thanks for improving .net, we appreciate it. We're having trouble internally keeping up with so many changes (Which is a lovely problem to have, we appreciate all the effort and don't want to discurage anyone from submitting PRs!), so we might take a bit until we can confidently merge any more .net PRs, at least until we understand how it all fits together now.

If you have any more PRs in the pipeline we'd appreciate if you could merge them all into one so we can take a look at it once we're ready to review them in a few weeks, thanks :)

@RubenRBS
Copy link
Member

Having said that, once you change the suggested thing in the config, this one looks good to merge

Copy link
Member

@RubenRBS RubenRBS left a comment

Choose a reason for hiding this comment

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

One more change for the config to avoid future issues

etc/config/csharp.defaults.properties Outdated Show resolved Hide resolved
Copy link
Member

@RubenRBS RubenRBS left a comment

Choose a reason for hiding this comment

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

Thanks!

@hez2010
Copy link
Contributor Author

hez2010 commented Dec 25, 2022

Hello, any chance to get this PR merged? It should improve the execution-only mode a lot by avoid running crossgen2.

@mattgodbolt
Copy link
Member

I'll take a look if not later today, then tomorrow! Thanks for your patience, appreciated your kind and gentle ping on this!

Copy link
Member

@mattgodbolt mattgodbolt left a comment

Choose a reason for hiding this comment

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

Sorry it took so long. The change to the code looks ok but I don't understand the reference to the non-existent nugetPackages directory. What is supposed to be there?

@@ -5,6 +5,7 @@ compilerType=csharp
defaultCompiler=dotnet700csharp

group.csharp.compilers=dotnettrunkcsharp:dotnet700csharp:dotnet6011csharp
group.csharp.nugetPackages=/opt/compiler-explorer/dotnet-nuget
Copy link
Member

Choose a reason for hiding this comment

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

I don;t see this directory on our network share:

Dec/26 22:51 admin-node~ $ ls -ld /opt/compiler-explorer/dotnet-*
lrwxrwxrwx  1 root root    21 Dec 26 07:35 /opt/compiler-explorer/dotnet-trunk -> dotnet-trunk-20221226
drwxr-xr-x 10 root root 22528 Dec 19 07:23 /opt/compiler-explorer/dotnet-trunk-20221219
drwxr-xr-x 10 root root 22528 Dec 20 07:31 /opt/compiler-explorer/dotnet-trunk-20221220
drwxr-xr-x 10 root root 22528 Dec 21 07:32 /opt/compiler-explorer/dotnet-trunk-20221221
drwxr-xr-x 10 root root 22528 Dec 22 07:31 /opt/compiler-explorer/dotnet-trunk-20221222
drwxr-xr-x 10 root root 22528 Dec 23 07:29 /opt/compiler-explorer/dotnet-trunk-20221223
drwxr-xr-x 10 root root 22528 Dec 25 07:37 /opt/compiler-explorer/dotnet-trunk-20221224
drwxr-xr-x 10 root root 22528 Dec 26 07:35 /opt/compiler-explorer/dotnet-trunk-20221226
drwxr-xr-x  3 root root  6144 Nov 25 07:21 /opt/compiler-explorer/dotnet-v6.0.1
drwxr-xr-x  9 root root 22528 Nov 25 23:00 /opt/compiler-explorer/dotnet-v6.0.11
drwxr-xr-x  9 root root 22528 Nov 25 22:59 /opt/compiler-explorer/dotnet-v7.0.0

What does this refer to?

Copy link
Contributor

@partouf partouf Dec 27, 2022

Choose a reason for hiding this comment

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

should probably be /opt/compiler-explorer/dotnet_nuget-v6.0.0

Copy link
Contributor

Choose a reason for hiding this comment

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

$ ls -l /opt/compiler-explorer/dotnet_nuget-v6.0.0/packages/
total 0
drwxrwxr-x 3 root root 28 Jan 31  2022 fsharp.core
drwxrwxr-x 3 root root 28 Jan 29  2022 microsoft.aspnetcore.app.runtime.linux-x64
drwxrwxr-x 3 root root 28 Jan 29  2022 microsoft.netcore.app.runtime.linux-x64

Copy link
Contributor Author

@hez2010 hez2010 Dec 27, 2022

Choose a reason for hiding this comment

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

I don't understand the reference to the non-existent nugetPackages directory.

It's reserved for the future. Currently we don't have the directory existing.
If we want to provide nuget packages support, we can create the directory from the infra in the future.
dotnet build won't fail for a non-existent nuget source.

Copy link
Member

Choose a reason for hiding this comment

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

Can we remove it please? There's no need to reserve anything for the future, we can make a change later to add it just as you've done here. Right now it's confusing and doesn't add anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks; testing locally now.

Copy link
Member

Choose a reason for hiding this comment

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

Worked! will merge. thanks for your patience.

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

Successfully merging this pull request may close these issues.

None yet

4 participants