Consider enhancing ModuleAttribute support or alternative approaches

[Skrypt]

Discussed in https://github.com/OrchardCMS/OrchardCore/discussions/11297

^{Originally posted by **mwpowellhtx** March 5, 2022} ### Opportunities... The opportunity at hand is to allow for easier versioning, and relaying of potentially other elements, via `csproj` _properties_. Specifically when we already have [`SemVer`](https://semver.org) versioning in the project file, we just want to pass that in. But we may also relay project properties for `Name`, `Author`, and so forth. ### Solutions... We use [automation](https://www.nuget.org/packages/BumpAssemblyVersions/) and project variables to convey key bits of details to various parts, including versions, authors, etc. We think that pipeline is best to connect dots with the `Orchard` `ModuleAttribute`. Better, of course, why not use the `Assembly` and `AssemblyName` attributes that already exist and have been well established in the .NET framework, but that is a potentially wider discussion. ### Alternatives... Considering `MSBuild` `AssemblyAttribute` for use in this case, although the docs are rather sparse where that is concerned. The most common use case I think is `InternalsVisibleToAttribute` which also has _params ctors_. Also, I do not think there is great support for setting actual _properties_ on such an `Attribute` instance, but I could be wrong about that. Crude project level _params_, stringification, etc, are supported, but I think [that would also mean adding _ctors_ to the `Attribute`](https://github.com/OrchardCMS/OrchardCore/blob/339d74b251b53393dd510f8ad3027aa17d78500d/src/OrchardCore/OrchardCore.Abstractions/Modules/Manifest/ModuleAttribute.cs), which is not a big deal provided `Orchard` can respond in a timely manner. We might even consider pushing a simple PR if that will help to bridge the immediate gap. Open to suggestions, feedback, concerns, questions. Thanks!

[Skrypt]

Okay, @jtkech correct me if I'm wrong but ModuleAttribute is internally used in Orchard Core to set versioning on modules. Though the version of modules inside OC features is not really displayed nor used. We generally rely on AssemblyVersion for our Nuget packages and also modules versions. Here, I think @mwpowellhtx needs direction on how he can achieve extending his own Orchard Core modules versioning so that it can be automated through the usage of an external library that will automate that process.

I'm referring this discussion to @jtkech because I know he worked on that part else I will need to read that code and analyze it to give you better answers. Though, my first impression is that using the Manifest ModuleAttribute would be wrong because we don't use the Version property anymore in OC. It was used in Orchard 1 when we allowed dynamic compilation on runtime.

[jtkech]

For now just for info, for modules/themes we have an msbuild script adding custom assembly attributes, ModuleMarkerAttribute to mark an assembly as a module, ModuleAssetAttribute to register static assets.

https://github.com/OrchardCMS/OrchardCore/blob/275ce1e6228c52016196a22163953470dcabe0b9/src/OrchardCore/OrchardCore.Module.Targets/OrchardCore.Module.Targets.targets#L67-L76 By code in Manifest.cs files we also use ModuleAttribute and FeatureAttribute to register Module / Theme metadata, some will be used at runtime as the dependencies between modules, others are for display only.

Look at OC.TheModule.AssemblyInfo.cs files under obj folders, alongside other attributes that may be defined in .csproj files as AssemblyCopyrightAttribute, look at OC.Commons.targets under the OC.Build folder for the common values we are using.

Then, we retrieve these data at runtime

https://github.com/OrchardCMS/OrchardCore/blob/275ce1e6228c52016196a22163953470dcabe0b9/src/OrchardCore/OrchardCore.Abstractions/Modules/Module.cs#L25-L41

You can inject IApplicationContext to retrieve from any module these data, the assembly, embedded files ...

_applicationContext.Application.GetModule(moduleName).Assembly;
_applicationContext.Application.GetModule(moduleName).AssetPaths;
_applicationContext.Application.GetModule(moduleName).GetFileInfo(fileSubPath);

Notice that all files, unless those under the Assets folder and other well known files to exclude, are automatically embedded in the modules / themes assemblies, could be useful to embed any custom info.

[Skrypt]

Ok, let's make a distinction now. When someone uses Orchard Core, it is precompiled and used as a Nuget package mostly. Really, rarely someone will be encouraged to use source code directly. So, what we are trying to define is how we should automate the versioning of custom-made modules or entire solutions made with OC Nuget packages references.

So when we build an Orchard Core module we need to make it reference the OrchardCore.Module.Targets or OrchardCore.Theme.Targets project so that it uses these to identify the assembly as one that can load features specified in the Manifest.cs file. These features can be versioned through the ModuleAttribute.cs parameters but it is not something I remember that we are still using. So, the solution mentioned by @mwpowellhtx seems to be using some automation to replace either .csproj or Properties.cs files which contain these assemblies details but I did not have time to look at it much. I believe that at this point this is only about understanding what his library does and analyzing how to integrate it inside the context of a "custom module" for example.

And of course, if any other project is available here on Github (free licencing) that does that for .NET Core would also make sense to consider. Here again the link to his project : https://github.com/mwpowellhtx/BumpAssemblyVersions

Normally if his project is a Visual Studio task then it has nothing to do with how we do things in Orchard Core but more about simply configuring it and using it. Though, if one wants to go down the path of using the versioning of the ModuleAttribute that would probably need some more thoughts.

[jtkech]

Yes that was For now just for info, will try to look at the mentioned project

[mwpowellhtx]

Per discussion with @sebastienros during today's triage, reviewed the opportunity at hand. Working on FeatureAttribute and ModuleAttribute constructors. Also as happenstance, extended a bit into FeatureProvider. All fairly straightforward, and should allow build pipeline scenarios involving csproj <AssemblyAttribute/> usage.

Although I have some questions re: Type whether that is not better suited to be something like an enum, i.e. perhaps enum FeatureKind { Module, Theme }, or what have you, based on cursory code inspection; however, leaving that nuance alone for the time being.

But there are a couple of opportunities in there to improve things which I will take advantage of the opportunity, in how Category and Priority are both coallesced via the FeatureProvider. Will also allow for semi-colon (';'), comma (','), or space (' ') delimited inputs for Tags and Feature Dependencies, which will be subsequently split. By convention I believe csproj tends to present semi-colon delimited elements, when that is the requirement to do so, so this works out nicely, even from csproj contexts.

There may be other use cases tucked away in there somewhere; i.e. perhaps a ThemeAttribute, along these lines. I'll scan for that tomorrow.

I expect to push and have ready in a commit squashed (?) PR in a day or so.

[mwpowellhtx]

Making progress. Updating through ThemeAttribute, also ModuleMarkerAttribute, allowing for the attribute specific use cases. Couple of new instances being created in my review, making the appropriate adjustments to those as well.

Anyone with any insights on a potential naming collision between OrchardCore.Modules.FeatureAttribute and OrchardCore.Modules.Manifest.FeatureAttribute? Was the OC attribute class intended to be an abstractions instead? Perhaps that is what it was before it matured via Manifest to support Modules, Themes, etc. I'll leave it alone for now, but I wonder it that was the intent.

Thanks...

[mwpowellhtx]

To clarify the above, I'm not suggesting OC adopt any automation; leaving that up to the maintainers' discretion. The dots that I do want to connect, as discussed during triage, are to expose pertinent constructors that we can access from csproj MSBuild steps as appropriate. For our own internal build pipelines, we need the constructor, the properties will not work, AFAIK. That's all. Re: scope beyond that, happy to weigh in there as well, but I just wanted to clarify that.

So, internally, as a Feature or Module author, we would declare our attributes in our csproj and automate our own versioning. Probably also relay additional bits, assembly names, company, author, whatever, as elaborated above. Most definitely drop the Manifest.cs, that source file becomes unncessary at that point.

Progress wise, working my way through the ModuleAttribute derivations. It is starting to look like a swiss army knife pattern of sorts; base Attribute class, with strands popping up here and there, Theme here, Asset there, so on and so forth. Not that there is anything wrong with that, but if the intention is not to leverage what makes it a ModuleAttribute, then I wonder if that could be streamlined, better factored, etc, at all. Of course at this point, the risk would be breaking changes to existing usages, so my aim is to mitigate those moments of opportunity best I can.

Edit: re: ModuleAssetAttribute above, the name is similar, but does not derive from ModuleAttribute after all. I am focused. Thanks!

And if I do not hit any major snags or concerns that need to be addressed, should be well within presenting a PR next week for triage.

Will visit all this next week during the Tuesday meeting.

[mwpowellhtx]

Moving along nicely. Have a couple of unit tests focusing on FeatureAttribute, ModuleAttribute. Will extend that for the other couple of known use cases as well, ModuleMarkerAttribute, ThemeAttribute, I think. Will also make an effort to include test projects that exercise the <AssemblyAttribute/> usage, which should help connect the dots examples which we wanted in the first place.

[ns8482e]

@mwpowellhtx not sure what’s benefit you’ll get after all this change?

IMO Module attribute version is only for you to stay in sync with OC major version, and you update manually only when you switch to next OC version if you wish ( or you can keep 1.0 forever it won’t have any effect in OC)

All module upgrade are really done by nuget so as long as you publish all your module and their updates to follow semver in csproj and build and publish nuget package via pipeline it should be enough.

[mwpowellhtx]

@ns8482e Well, again, we're not talking about the internal OC build pipelines, as I stated numerous times. We are talking about OC feature/module authors' build pipelines. When my assemblies bump versions, that should be reflected by the MODULE ASSEMBLY 'version'. That is a completely separate issue from OC itself.

For that matter, it seems extremely redundant to conventional MSBuild supported AssemblyInfo.cs attributes, for that matter. Things like 'author', 'description', all very well supported by the MSBuild attribute discovery, AFAIK. Has been that way for a long, long time. No need for OC or anyone else to reinvent that wheel.

Now, tangentially, I completely understand and respect the OC framework discovering assemblies to be a 'module' or a 'theme' or what have you. Insofar as that aspect can be conveyed by an appropriate attribute, yes, I totally 'get' that.

Can I clarify that further for you?

[mwpowellhtx]

Finding that in the targets, ModuleMarkerAttribute is being auto-generated, as well as a separate ModuleAssetAttribute for that matter. What was the purpose for these? That a FeatureAttribute or ModuleAttribute would not be sufficient?

Probably already discussed, number one, but the fact that these are auto-generating is causing potentially ambiguous opportunities discovering ModuleAttribute. Number two, also leaves module authors prone, IMO, to spoofing, counterfieting, or other fraudulent activities, if you ask me. Again, I think ModuleAttribute does everything ModuleMarkerAttribute needs to do without reinventing any wheels there.

Thoughts? Concerns?

[ns8482e]

@mwpowellhtx I understand but my question is simple what benefit you get out of these change and what benefit the community get?

NONE

I guess version properly from module attribute is nowhere used after dynamic compilation was removed

all version upgrades for modules are really based on nuget package version

[mwpowellhtx]

Key scenario verifying integration at a CSPROJ level, introducing the <AssemblyAttiribute/> use case, dropping the Manifest.cs usage. Not dissimilar to the baseline OrchardCore.Tests.Modules assemblies.

After unit testing the attributes themselves, verifying language level ctors, arguments, etc. That all went pretty smoothly. But when I made the leap into a more real-world use case scenario, started to implode on what looks like possibly a faux premise in the ModuleMarkerAttribute auto-generated targets.

protected virtual T[] GetAssemblyAttributes<T>(Type rootType)
    where T : Attribute =>
    rootType.Assembly.GetCustomAttributes<T>().ToArray();

protected virtual T GetAssemblyAttribute<T>(Type rootType)
    where T : Attribute
{
    var attribs = GetAssemblyAttributes<T>(rootType);
    Assert.Single(attribs);
    return attribs[0];
}

rootType being any anchor type that allows us to glean the rootType.Assembly, from which we should be able to extract the Attributes. Plural, and singular, which, if I understand the ModuleAttribute usage, should only appear once. It 'does' but a second ModuleMarkerAttribute instance is also appearing there.

[AttributeUsage(AttributeTargets.Assembly, AllowMultiple = false, Inherited = false)]
//                                         ^^^^^^^^^^^^^^^^^^^^^
public class ModuleAttribute : FeatureAttribute
{
    // ...
}

What purpose is ModuleMarkerAttribute serving that ModuleAttribute does not already serve? Besides perhaps allowing for authors to specify custom Type? Which ModuleAttribute could support given an apporpriate ctor, without sacrificing the other details.

Scanning the solution for appearances of ModuleMarkerAttribute, I see no appreciable usage there along these lines. But perhaps someone could clarify that for me?

@jtkech This one possibly attributed to your concerns, sir, if I read the git blame history accurately? Could you offer some insight? Would a Manifest.cs or csproj ctor overload not work for your case? Certainly I am not finding anything in the current baseline code version that would, in my mind, justify a separate class, let alone targets generating what is effectively a second ModuleAttribute instance. Let me know ASAP, thank you, sir.

[mwpowellhtx]

@ns8482e Not 'none'. Rather, ability to relay $(AssemblyVersion) at build time, for starters, after having been bumped. Also other naturally occurring CSPROJ properties, perhaps a $(Company) (i.e. author) or $(Description), maybe also $(PackageTags) (i.e. tags), along these lines.

Have been very clear by that, as have others.

'None' of which is possible today without the corresponding ctors.

@sebastienros If there is a better idea than that, by all means, please do add constructive feedback.

Thank you.

[ns8482e]

Correct, it’s not possible today but my question is - even if by making this change and it’s possible tomorrow then what value/benefit you got out of this?

You answered what you can achieve but you didn’t answer WHY

[mwpowellhtx]

@sebastienros Ad nauseum have I (and others) explained it. Kindly, read for comprehension, please. @ns8482e

[mwpowellhtx]

On encouragement during today's meeting, I went ahead and pursued a MSBuild CSPROJ driven FeatureAttribute target. Preliminary test results are promising, it works pretty well. Turns out this was a fairly simple thing to go ahead and achieve, so I will also include that in my push forthcoming next day or so.

Assuming the full FeatureAttribute ctor:

public FeatureAttribute(
    string id
    , string name
    , string category
    , string priority
    , string description
    , string featureDependencies
    , object defaultTenant
    , object alwaysEnabled
)
{
    // ...
}

In draft form; also included messages, conditions, etc, but for doc purposes here:

<Target Name="EmbedFeatures" AfterTargets="AfterResolveReferences">
  <ItemGroup>
    <AssemblyAttribute Include="OrchardCore.Modules.Manifest.FeatureAttribute">
      <_Parameter1>@(OrchardCoreFeatures)</_Parameter1>
      <_Parameter2>%(OrchardCoreFeatures.Name)</_Parameter2>
      <_Parameter3>%(OrchardCoreFeatures.Category)</_Parameter3>
      <_Parameter4>%(OrchardCoreFeatures.Priority)</_Parameter4>
      <_Parameter5>%(OrchardCoreFeatures.Description)</_Parameter5>
      <_Parameter6>%(OrchardCoreFeatures.Dependencies)</_Parameter6>
      <_Parameter7>%(OrchardCoreFeatures.DefaultTenant)</_Parameter7>
      <_Parameter8>%(OrchardCoreFeatures.AlwaysEnabled)</_Parameter8>
    </AssemblyAttribute>
  </ItemGroup>
</Target>

And in usage, something like this, basically aligned with the fully parameterized ctor:

<ItemGroup>
    <OrchardCoreFeatures Include="OrchardCore.FeatureSet.Alpha" Name="Alpha" Category="three" Priority="4" Description="five" Dependencies="six;seven;eight" DefaultTenant="true" AlwaysEnabled="true" />
    <OrchardCoreFeatures Include="OrchardCore.FeatureSet.Bravo" Name="Bravo" Category="four" Priority="5" Description="six" Dependencies="seven;eight;nine" DefaultTenant="true" AlwaysEnabled="true" />
    <OrchardCoreFeatures Include="OrchardCore.FeatureSet.Charlie" Name="Charlie" Category="five" Priority="6" Description="seven" Dependencies="eight;nine;ten" DefaultTenant="true" AlwaysEnabled="true" />
</ItemGroup>

If the properties are not defined, I think the result would be empty string or even null; would have to test a couple of cases where this is the issue. Where that might be problematic, i.e empty Priority, or the boolean args, but I think the conversions and so forth are probably robust to that being the case. Dependencies, typically semi-colon delimited since that is typically the MSBuild convention, but also supporting comma or space as appropriate.

Again, ultimately, the idea is to not only better utilize existing or user defined CSPROJ properties, but, I think, also move away from Manifest.cs stuff altogether, up to dev discretion obviously.

[mwpowellhtx]

Bit late in reporting today, but I think I have a viable approach extending into MSBuild Item. Tried to tinker with a bit, if there is a better way to filter and case select out Features from Modules from Themes from user-specific Types, I'm open to those sort of suggestions, in terms of perhaps consolidating the Item Lists, but otherwise, seems to work pretty well. In the meanwhile, I support containing them from items named OrchardCoreFeatures, OrchardCoreModules, and so on. User may specify a Type property on each OrchardCoreModules instance accordingly, or leave it unspecified for the default.

While I am there, I will also allow for variability in the Before- and AfterTargets. Would be fantastic to learn the actual MSBuild target at which moment the AssemblyInfo.cs is generated, i.e. drawing from <AssemblyAttribute/> and other elements. That should be a proper BeforeTarget for sure. But given a proper OrchardCoreEmbeddingAfterTargets and OrchardCoreEmbeddingAfterTargets property there, that allows authors to append their own build pipeline targets accordingly, as well, i.e. bump versions prior to rolling up all of the assembly information.

Usage examples:

<ItemGroup>
  <OrchardCoreFeatures Include="$(MSBuildProjectName).One"
                       Category="two"
                       Priority="3"
                       Description="four"
                       Dependencies="five;six;seven"
                       DefaultTenant="true"
                       AlwaysEnabled="true" />

  <OrchardCoreFeatures Include="$(MSBuildProjectName).Two"
                       Category="three"
                       Priority="4"
                       Description="five"
                       Dependencies="six;seven;eight"
                       DefaultTenant="true"
                       AlwaysEnabled="true" />

  <OrchardCoreModules Include="$(MSBuildProjectName)"
                      Type="three"
                      Category="four"
                      Priority="5"
                      Description="six"
                      Author="seven"
                      Version="8.9.10"
                      Website="oc://modules.prop"
                      Dependencies="ten;eleven;twelve"
                      Tags="eleven;twelve;thirteen"
                      DefaultTenant="true"
                      AlwaysEnabled="true" />
</ItemGroup>

Need to add a couple of unit tests for the Theme approach as well, should be fairly similar, I think, however, with similarly supported targets.

<ItemGroup>
  <OrchardCoreThemes Include="$(MSBuildProjectName).MyTheme"
                      Base="Base.Theme"
                      Category="four"
                      Priority="5"
                      Description="six"
                      Author="seven"
                      Version="8.9.10"
                      Website="oc://themes.prop"
                      Dependencies="ten;eleven;twelve"
                      Tags="eleven;twelve;thirteen"
                      DefaultTenant="true"
                      AlwaysEnabled="true" />
</ItemGroup>

Included a bit of error detection as well for obvious use cases, should not have more than one Module or Theme, respectively. Additionally, should not have a Module and a Theme declared in the same assembly, if I understand the OC architecture correctly. Theme is of course also a Module where that is concerned. Will need to clarify that. There may be any number of Features, of course.

Also to reiterate, direct <AssemblyAttribute/> usages also works fantastically well.

Pending pulling together the unit tests around these cases, contain in a branch, and make the PR as early as possible tomorrow. If possible approaching tomorrow's triage.