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

MSI Bundle generator should honor msbuild Authors property when setting the authors value of the resulting msi file. #8260

Closed
1 of 2 tasks
AraHaan opened this issue Dec 14, 2021 · 15 comments · Fixed by #8275

Comments

@AraHaan
Copy link
Member

AraHaan commented Dec 14, 2021

  • This issue is blocking
  • This issue is causing unreasonable pain

For starters, let's say for example an .NET Foundation project uses the arcade to produce Runtime and Reference msi bundles. The resulting bundles incorrectly state Authors: Microsoft Corporation instead of Authors: .NET Foundation for example (in case the user set the msbuild property named Authors to .NET Foundation (which I think some people do). However there is also 3rd parties who use the arcade to produce msi's and while it might be ok for thinks to hardcode it to Microsoft Corporation I do not think it is a good idea to hard code it as when 3rd parties use arcade to produce said msi installers, users who do not know who actually made said bundles might incorrectly assume Microsoft made them and that they are "safe" to install.

This is why I find this an issue as well, anyone can add the arcade feeds into their nuget feeds and use the arcade packages to produce installers for their 3rd party code with ease. Due to that, it is never really a good idea to hard code this to begin with. This is both an issue of Trust for end users (as they would think that Microsoft made it and that it is free from malicious code (which might not always be the case)), as well as an issue where Microsoft incorrectly gets credit for things they did not make / fully own (as far as I am aware of).

@dleeapho
Copy link

cc @joeloff @NikolaMilosavljevic

@joeloff
Copy link
Member

joeloff commented Dec 17, 2021

The fields are backed by variables, e.g.

But the definitions like

<?define Manufacturer = "Microsoft Corporation" ?>

should probably be using <?ifdef?> tests. I know why they don't because these all came from internal builds. The arcade tooling is limited to what can be built as they were designed for very specific MSIs for .NET.

To be fair, I have seen external requests come in before from developers wanting to leverage parts of Arcade. While Arcade is open, I'm not sure that its original goal was to create a generic build system. There are many parts of Arcade that integrated with non-open software like Visual Studio. and would be of little value to external developers for now.

I'd like for @mmitche or @markwilkie to weigh in here.

@AraHaan
Copy link
Member Author

AraHaan commented Dec 17, 2021

To be fair, I have seen external requests come in before from developers wanting to leverage parts of Arcade. While Arcade is open, I'm not sure that its original goal was to create a generic build system. There are many parts of Arcade that integrated with non-open software like Visual Studio. and would be of little value to external developers for now.

That also depends on what those external developers are making as well. Some people want to make their own .NET Runtimes (and crossgen2 specific things to make them faster) and possibly Integrate them into Visual Studio with an separate integration package.

As such even those developers would get upset if arcade stamps their packages with Microsoft Corporation instead of <insert company name here> that made the packages (I mean who would not be upset about that one).

Another example I would use would also be Paint.NET knowing that (assuming rick wanted to reduce the need to force the program to be SCD and do this to make it FDD):

  • it contains a public API to use for making plugins.
  • the public APIs contain some low level native bits for it's implementation (and the person wants to ship the main application as AnyCPU but not sacrifice startup times by splitting much of the things it does into it's own public apis and runtime packages specially optimized per cpu and so that way the apphost would just do the right thing and load the right one).
  • it gets updated every so often.
  • possible for plugins to have a copy of an old version of those public API binaries in their own folders (and possibly get loaded into a state that brings down the program aka dll hell).
  • other reasons not yet listed.

All of those would generally be solved with shipping those public apis as a runtime msi package (that can be installed in their DOTNET_ROOT and is crossgen2'd). This would solve this because now then none of the public api binaries would get copied on build unless they publish the plugins as self contained).

@mmitche
Copy link
Member

mmitche commented Dec 17, 2021

I'm fine with allowing these to be properly overridden.

@markwilkie
Copy link
Member

Sounds good - I'm fine w/ this change too. Is the idea for you to put up a PR @AraHaan ? (please note that most of us are about to be out until the new year)

@AraHaan
Copy link
Member Author

AraHaan commented Dec 17, 2021

I made the PR as draft until I can check if there are no tests for the Installers package (As I am unsure about that one).

@AraHaan
Copy link
Member Author

AraHaan commented Dec 18, 2021

The change works, just for some reason it seems that when using the msi's from this change to produce an exe bundle, it then still has the issue of setting the copyright on it to Microsoft Corporation with no way to change it (or even the installer background image). However I came close to getting it set up properly on my end.

@AraHaan
Copy link
Member Author

AraHaan commented Dec 20, 2021

@mmitche, @markwilkie should the PR be good to merge for now and then look into the exe bundle issue later that I posted above when there is more time after the new year?

@mmitche
Copy link
Member

mmitche commented Dec 20, 2021

Okay. Let's have @joeloff take a quick look and then it should be good to merge.

@markwilkie
Copy link
Member

Thanks @AraHaan !

@AraHaan
Copy link
Member Author

AraHaan commented Dec 21, 2021

Welp turns out my changes also magically applies to exe bundle installers so all seems to be good on that part.

My only issue now is with the part where I cannot force it to use a different installer background image on Windows.

@joeloff
Copy link
Member

joeloff commented Dec 21, 2021

You'd at least need to override this, but there's probably more things that need to be customized, e.g. the theme files are very much geared for .NET, as are some messages that have been added in the .wxl files.

@AraHaan
Copy link
Member Author

AraHaan commented Dec 21, 2021

Yep, there would need to be quite a bit of changes.

@markwilkie
Copy link
Member

Is this still something you'd like to continue working on @AraHaan ?

@AraHaan
Copy link
Member Author

AraHaan commented Jan 13, 2022

yes, I even commented on the PR.

NikolaMilosavljevic pushed a commit that referenced this issue Jan 14, 2022
…ng the authors value of the resulting msi file. (#8275)

Fixes #8260.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants