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

First step toward a general recipe for engine and extensions #1426

Merged
merged 3 commits into from
Jul 2, 2024

Conversation

CharliePoole
Copy link
Collaborator

Fixes #1422

Initially, I planned to modify the existing scripts incrementally in order to generalize them. This proved a bit difficult and I eventually switched to creating a simplified version of my TestCentric recipe, which seems like a good starting point for a standard recipe.

As compared to the TestCentric recipe, this one is simplified by removing a number of experimental and alternate approaches to doing the same thing. BTW, I created the old scripts being replaced here were as part of my early experiments, which eventually led to that recipe. Full circle.

As compared to TestCentric, this recipe has a few extra features, primarily related to making it more convenient to deal with the large number of packages created by this solution. I've given us a -where option to simplify working on a single package and I've saved the summaries of package tests as text files to reduce the need to scroll.

@nunit Naturally, I appreciate any feedback you may have but I'm particularly looking for some review that address whether you think the recipe is likely to provide convenience. Frankly, it's the main factor in my being able to maintain 18 TestCentric repos on my own.

I suggest starting with the usage message in help-messages.cake and at the structure of build.cake

@OsirisTerje @rprouse @mikkelbu I'd particularly like to hear from those who are very familiar with cake!

Next steps after merging this:

  1. Make it work with at least one extension. I know one new feature is needed: support of multiple console versions.
  2. Make this a separate project for loading into scripts.

@CharliePoole
Copy link
Collaborator Author

Failing merge is due to a continuing issue in AppVeyor. Please review anyway. If someone wants to download and try it that would be great!

@mikkelbu
Copy link
Member

@CharliePoole I can take a look at it, but probably first tomorrow night or Sunday night as my days are packed (and I've not done much with cake the last couple of years).

Is it on purpose that the output folder is included in this PR?

@CharliePoole
Copy link
Collaborator Author

@mikkelbu No, I changed output to package in .gitignore, but I neglected to remove the directory. :-(

@OsirisTerje
Copy link
Member

There are a lot of code in those cake files. In NUnit I started to move the scripts into CSharp projects (there is only a version parser there now). That way it was possible to test them. Cake has no problems consuming .cs files.

Since the cake scripts are mostly, well scripts, I have struggled with them, so moving them to something that can be tested seems to me to solve some of the issues I have had with cake.

Make this a separate project for loading into scripts.

You might point to that here ?

I am also a bit concerned if it is actually necessary. It might be, but this looks like a very complex piece of deployment code.

However, having recipes (or templates) is a good way to make sure one does things the same way, so I am positive there.

With regard to the Console/Engine project, I would like to see it simplified (imho), so that we can reduce the number of moving parts (projects/components) there.

I don't think we need a lot of different options on how to run this. I would prefer most 2, one for development and one for release (even locally), and having build do the one thing, and build -t package do the second have worked very well for me.
I also have had no issues running all the tests. Actually, I would prefer using ordinary dotnet test for doing that, and not having to use the cake build for that.

Currently we have different approaches to building and deploying in all projects, analyzer, framework, adapter and console/engine. As long as different people work on these, that is no problem, but once I was doing both the adapter and the framework deployment, I started to struggle. When I mixed in the console/engine, which had a very prescriptive way, I struggled with release notes, getting the builds up to nuget and so on. I knew that for any release of the console/engine, I did need time and space to do that right. It did work, but we had one version that didn't go as it should.


Looking at the help-messages.cake:

There is a new nomenclature here, which concerns me. Tests are specified with --level, which seems to indicate where the builds are being done (like in a PR). I don't understand the effect of these. Which test projects do they run? The --trace command is called verbosity elsewhere, or I might misunderstand this. The options --nobuild and --nopush but no commands for the opposite leads me to believe the default is to both build (compile) and push the package up. The dotnet way is restore, build, pack and push, where the later ones implies the earlier if a -noXXXX is not present. Isn't that a good way to do it?

Looking at the build.cake:

I see there are a structure there, lots of structures actually. I am not able to get the grasp of all of it though. It looks very different from the other cake scripts we have. It is probably an improvement, but it would be good with a design description of this.

@CharliePoole
Copy link
Collaborator Author

@OsirisTerje Some general comments re your comments... no conclusion. :-)

Cake vs C#

Cake is, of course c#. It's indeed possible to use the .cs extension, which gives you a bit of intellisense, but not as much as one might like. That's because it's all embedded in a class created by cake, which is invisible to the intellisense. I would favor converting the .cake files to .cs but was trying to keep them consistent with the framework's use of cake.

New Code / Structures

A lot or what seems new to you is actually old, but was a bit more hidden in the past. The original script had 12 cake files and with this issue we have 30. Some of this is due to splitting existing files so they perform a single function, but others are additions of functionality, e.g.: the help message, the infrastructure needed to allow a project to modify the standard tasks. The number of tasks has been reduced somewhat in this change. The structures used to define packages have always existed but were not in the main build.cake file. My objective was to put everything that is specific to nunit-console into build.cake and have the included cake files be the same for all affected projects. My scope, of course is only nunit-console and the engine extensions.

Audience

As you say, I think it comes down to the question of who will be using this. Our re-organization a number of years back implied that different people would manage different projects. That happened for a while but is no longer happening. In the short term, I assumed I would be creating new releases but that won't last forever and it doesn't seem that the engine is a very attractive job for most existing members of the organization.

Other Stuff

You asked me to point to something showing how a separate project can be used to house the scripts. Of course, that doesn't exit for nunit yet, but it would basically mean replacing the line in build.cake...

#load cake/*.cake

with a line line this one...

#load nuget:?package=NUnit-Console.Cake.Recipe&version=1.2.3

(You can see this working in any of the TestCentric projects.)

I think it's simpler to do the first two projects using cut and paste, before moving to that approach.

In the longer term, it could make sense to abandon cake in favor of direct use of the dotnet CLI, but that kind of change requires a broader participation of folks who will be using the scripts.

@CharliePoole
Copy link
Collaborator Author

@OsirisTerje @mikkelbu @rprouse [sorry if I missed anyone else who is "into" cake]

I'd like to suggest looking at this a bit differently.

We already have extensive cake tooling, starting from it's introduction by @rprouse, who has also contributed to the cake project. I took it a bit further in TC, going the path of a cake recipe, with some help from other people.

Because each project that uses cake (nunit console, framework and extensions to my knowledge) a slightly different approach has been taken - different people at different times. The existing cake scripts for console and extensions represent my own very early work with cake.

This PR presents a lot of moving parts, more than what was already there, but I think a bit more organized. But on the scale of all usage of cake by NUnit, there are fewer moving parts in total. In fact, started this because I found it confusing to switch from console to various extensions and have a different set of options and targets for each one. I'm pretty sure it will help me for the period of time I continue to collaborate on the project.

So what I'm suggesting is that we not look at this in light of whether it uses anyone's preferred technology for CI but whether it makes an incremental improvement in what we already have.

Based on @OsirisTerje 's comment about a compiled solution, I'm looking at converting the recipe to a console app using
Cake Frosting project, incorporating the recipe as C# code. [Cake nomenclature makes it hard to google without getting actual cake recipes!]

cake/banner.cake Outdated Show resolved Hide resolved
cake/build-settings.cake Outdated Show resolved Hide resolved
cake/build-settings.cake Outdated Show resolved Hide resolved
cake/build-settings.cake Outdated Show resolved Hide resolved
string solutionFile = null;

if (System.IO.File.Exists(Title + ".sln"))
solutionFile = Title + ".sln";
Copy link
Member

Choose a reason for hiding this comment

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

I would just return here directly instead of do the single return at the bottom. You're already using multiple returns in CalcPackageTestLevel so there's precedent for it, and this way makes it look like there might be other "processing" of solutionFile before returing.

cake/build-settings.cake Outdated Show resolved Hide resolved
Copy link
Member

@veleek veleek left a comment

Choose a reason for hiding this comment

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

Submitting a few code comments, have not finished reviewing a large portion of this, I'm done the first ~10 files.

@CharliePoole
Copy link
Collaborator Author

@veleek Thanks for the review. I realize this was a very large change. I liked all your suggestions and they are reflected in the last commit.

I'll wait to see if there are other reviews and for a final word from @OsirisTerje who had some doubts about this approach.

@CharliePoole
Copy link
Collaborator Author

@OsirisTerje To avoid later merge problems, I'd like to either get this in or delete it asap. I have two or three branches waiting to either merge this change or not before I creatine PRs for them.

@OsirisTerje
Copy link
Member

@CharliePoole Just proceed. I just wanted to raise my doubts :-) You have read them, and then I am fine.

@CharliePoole CharliePoole merged commit c4d49d0 into main Jul 2, 2024
4 of 5 checks passed
@CharliePoole CharliePoole deleted the issue-1422 branch July 3, 2024 16:21
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.

Standardize scripts across console and extension repos
4 participants