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

Add standalone template for experiments that don't need runtime features #238

Merged
merged 7 commits into from
Oct 21, 2020

Conversation

safern
Copy link
Member

@safern safern commented Oct 16, 2020

Fixes: #88

This is the template branch for new experiments to use in order to create an experiment that doesn't depend on runtime or framework features.

Make sure to ignore the first commit as that is just pulling all the arcade files into eng/common.

I will add a subsequent PR to enable official builds and to add links within the repo in the documentation I added in the README file.

If you think the branch name doesn't make sense, please feel free to suggest one, I'm not great and naming and standalone-template was the best I could come up with.

FYI: @pgovind @eerhardt @danmosemsft

@safern safern force-pushed the AddStandaloneTemplate branch 6 times, most recently from 56576ac to 82ce1c0 Compare October 16, 2020 19:56
@safern safern marked this pull request as draft October 16, 2020 19:57
@safern safern force-pushed the AddStandaloneTemplate branch from 82ce1c0 to fd39bc2 Compare October 16, 2020 20:22
@safern safern marked this pull request as ready for review October 17, 2020 00:15
@safern safern requested review from jkotas, joperezr and ericstj October 17, 2020 00:16
README.md Outdated Show resolved Hide resolved
Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

LGTM. It may be a good idea to get somebody intimately familiar with Arcade, etc. to review the build scripts.

@safern
Copy link
Member Author

safern commented Oct 17, 2020

I believe, the guys I tagged + @ViktorHofer would be very familiar and could provide good feedback for this. Thanks, @jkotas

@safern safern force-pushed the AddStandaloneTemplate branch from 3d309c4 to a8052dd Compare October 17, 2020 00:59
Copy link
Member

@joperezr joperezr left a comment

Choose a reason for hiding this comment

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

Left minor comments but looks great otherwise!

eng/Build.props Outdated Show resolved Hide resolved
eng/Versions.props Show resolved Hide resolved
global.json Show resolved Hide resolved
src/Experiment.csproj Outdated Show resolved Hide resolved
eng/pipelines/runtimelab.yml Show resolved Hide resolved
@jkoritzinsky
Copy link
Member

cc:@AaronRobinsonMSFT and @elinor-fung since we own the only experiment currently in this category to my knowledge.

src/Experiment.csproj Outdated Show resolved Hide resolved
@ViktorHofer
Copy link
Member

Curious, how do we manage changes from dotnet/runtime? Ie the testing infrastructure in this branch will soon be outdated.

@safern
Copy link
Member Author

safern commented Oct 19, 2020

Curious, how do we manage changes from dotnet/runtime? Ie the testing infrastructure in this branch will soon be outdated.

what do you mean with the testing infrastructure?

I just used the runsettings target logic, but we don't use the rest of the dotnet/runtime infrastructure, in fact this doesn't have anything from dotnet/runtime.

Experiments that do need a shared framework are forked off from runtime-master branch which is mirrored on every commit that goes into dotnet/runtime.

@ViktorHofer
Copy link
Member

I just used the runsettings target logic, but we don't use the rest of the dotnet/runtime infrastructure, in fact this doesn't have anything from dotnet/runtime.

Right, I'm talking about the runsettings.targets and .runsettings files which I'm changing in my VSTest PR in dotnet/runtime. Who would be responsible to update them here as well, ie to benefit from improvements in the VSTest runner?

@safern
Copy link
Member Author

safern commented Oct 19, 2020

That would be me or @joperezr I guess... Do you expect this target to change often?

@ViktorHofer
Copy link
Member

That is used when people want to use a custom runtime version... so in order to support running with dotnet test.

You could pass that information in via a CLI switch as Arcade's Test target should allow specifying additional CLI options. Or is this to support VS Test Explorer as well? Did you try if that works? I would assume that you need to generate an initial .runsettings file (same as what we do in pretests.proj) that is used, before the test app is built.

@safern
Copy link
Member Author

safern commented Oct 19, 2020

You could pass that information in via a CLI switch as Arcade's Test target should allow specifying additional CLI options. Or is this to support VS Test Explorer as well? Did you try if that works? I would assume that you need to generate an initial .runsettings file (same as what we do in pretests.proj) that is used, before the test app is built.

Yes, this is also to support VS Test Explorer when using a custom runtime version other than the one that comes with the SDK specified in global.json.

Yes I did try this on a clean build by opening the solution... the way I did it was by setting the GenerateRunSettingsFile as an InitialTarget in test projects, so it will produce it first thing no matter what target is ran.

@ViktorHofer
Copy link
Member

Yes I did try this on a clean build by opening the solution... the way I did it was by setting the GenerateRunSettingsFile as an InitialTarget in test projects, so it will produce it first thing no matter what target is ran.

When I added the VS Test Explorer support in dotnet/runtime, this didn't work. VS Test Explorer tried to load the .runsettings file specified in the property immediately (before a target ran) and failed. Keep an eye out for that to be certain that the right .runsettings file and SDK is used.

@safern
Copy link
Member Author

safern commented Oct 19, 2020

When I added the VS Test Explorer support in dotnet/runtime, this didn't work. VS Test Explorer tried to load the .runsettings file specified in the property immediately (before a target ran) and failed. Keep an eye out for that to be certain that the right .runsettings file and SDK is used.

I'll double check that. Thanks for the heads up.

@jkoritzinsky
Copy link
Member

So after looking through this from the consuming side, it seems significantly more complicated than what DllImportGenerator (which is the only current standalone project) would need. The inclusion of the platform-matrix.yml style of templating is significantly more complex than what we need for our project. We were planning on writing a simple yml entry-point that's easy to understand without multiple template layers on top of the Arcade ones. We don't need any of the complicated dotnet/runtime pipeline infrastructure for our build.

In fact, a simple matrix: object on top of the basic Arcade templates would satisfy our use case completely.

I'm a little concerned that this pipeline was designed without any input from the only standalone project today that might use it (I stumbled on this PR randomly, no one on the Interop team working on the DllImportGenerator project was tagged). I'm also worried that since we might not have any standalone projects using it for a while, it might atrophy and break before anyone adopts it.

I think if the .NET Runtime Infra v-team (preferably someone other than myself) commits to owning this infra and maintaining it in experiments like the DllImportGenerator experiment, I'll be more amenable to this design.

@eerhardt
Copy link
Member

which is the only current standalone project

The Microsoft.Data.Analysis package from corefxlab will also be using this template.

@safern
Copy link
Member Author

safern commented Oct 19, 2020

I'm a little concerned that this pipeline was designed without any input from the only standalone project today that might use it (I stumbled on this PR randomly, no one on the Interop team working on the DllImportGenerator project was tagged). I'm also worried that since we might not have any standalone projects using it for a while, it might atrophy and break before anyone adopts it.

Sorry that I didn't tag you guys. My understanding was that the DllImportGenerator was going to evolve into a experiment that needed runtime features, and wasn't going to use this template, that's why I didn't tag you here.

I will reach out offline, and this are valid concerns, so I will reach offline and get consensus on this from the interop team to meet your guys need as well.

@jkoritzinsky
Copy link
Member

Yes, we'll eventually move to getting runtime features, but that's planned for closer to the end of the experiment. Many of the APIs we need we can initially provide (with less performance) outside of the runtime. We don't want to fork the runtime into our branch (and significantly complicate our workflow) until we absolutely cannot avoid it.

@safern safern force-pushed the AddStandaloneTemplate branch 6 times, most recently from 7ecf460 to ba879fc Compare October 21, 2020 20:49
@safern safern force-pushed the AddStandaloneTemplate branch from ba879fc to ad569a7 Compare October 21, 2020 20:54
@safern
Copy link
Member Author

safern commented Oct 21, 2020

Thanks for the discussion and reviews. I've addressed/validated all concerns. This is ready to go, could you guys take another look please?

@safern safern merged commit 0809d81 into standalone-template Oct 21, 2020
@safern safern deleted the AddStandaloneTemplate branch October 21, 2020 23:15
jkoritzinsky pushed a commit to jkoritzinsky/runtime that referenced this pull request Sep 20, 2021
…ures (dotnet/runtimelab#238)

* Add arcade common files

* Add standalone library template for experiments that don't need runtime features

* Add documentation to README.md

* PR Feedback.

Co-authored-by: Jan Kotas <[email protected]>

* Quick fixups in README.md

* PR Feedback

* Simplify yml templates

Co-authored-by: Jan Kotas <[email protected]>

Commit migrated from dotnet/runtimelab@0809d81
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.

7 participants