Skip to content

[WIP] RPM packaging doesn't flag .NET Core DLLs as executables#64

Open
clemensv wants to merge 51 commits intoquamotion:masterfrom
clemensv:issue61
Open

[WIP] RPM packaging doesn't flag .NET Core DLLs as executables#64
clemensv wants to merge 51 commits intoquamotion:masterfrom
clemensv:issue61

Conversation

@clemensv
Copy link

[Discussion item. Not yet ready to merge]

PR to resolve issue #61 - proposes a range of changes to improve the accuracy of the test scenarios that read back existing RPM package by taking their metadata into account, introduces explicit archive file types for .NET assemblies and documentation files, adds detection for .NET assemblies, and adds a metadata extension to explicitly flag documentation files.

Clemens Vasters added 4 commits August 11, 2018 04:31
…ET assemblies as "mono". Removed flagging of all non-executable files as docs, which leads to breaking installs on container platforms.
@qmfrederik qmfrederik changed the title Issue61 [WIP] RPM packaging doesn't flag .NET Core DLLs as executables Aug 13, 2018
RPMFC_ELF32 = 1 << 0,
RPMFC_ELF64 = 1 << 1,
RPMFC_ELFMIPSN32 = 1 << 2,
RPMFC_PKGCONFIG = 1 << 4,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we keep these enum values? They map to the values defined in the RPM API I would prefer to keep them in sync.

Copy link
Author

Choose a reason for hiding this comment

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

I did some research before I pulled those values out.

The values you have here got dropped from RPM in 2010 and RPM's classification model doesn't use them anymore but rather uses the text classifier dictionary. My edits align to the current rpmfc.h header.

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK, sounds good then!

{
try
{
var assemblyName = AssemblyLoadContext.GetAssemblyName(fileName);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This try/catch sounds very expensive, and we can probably apply some more logic to determine whether a file is a .NET executable.

Copy link
Author

Choose a reason for hiding this comment

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

Exceptions are worrisome if they're on the hot path for performance sensitive code that runs on a sustained basis, like inside an ASP.NET app. This here is done once for each file during packaging of an app, so I don't find that of concern. The alternative is to pull in PE file decoder code, which is far more code (I started with that approach and discarded it). We could safeguard the section with a simple probe for the 'MZ' magic word being the first two bytes of the file content which should catch most cases.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We could safeguard the section with a simple probe for the 'MZ' magic word being the first two bytes of the file content which should catch most cases.

That sounds like a good compromise.

Copy link
Author

Choose a reason for hiding this comment

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

I'll add that.

qmfrederik and others added 23 commits August 13, 2018 18:36
Route pre/post install/remove scripts from RPM task to packager
Added installs script on RpmTask
Update Packaging.Targets.targets
Use NerdBank.GitVersioning to assign build numbers
Don't start branch builds if a PR is active
Build the demo project as part of a CI run
Bump libicu versions to add support for Ubuntu 18.04
qmfrederik and others added 24 commits August 31, 2018 14:46
Add integration tests, using Docker
RPM: Only flag files in /usr/share/doc as Doc
…ET assemblies as "mono". Removed flagging of all non-executable files as docs, which leads to breaking installs on container platforms.
@qmfrederik
Copy link
Collaborator

@clemensv The diff got a bit large (I guess because you merged master instead of rebasing), but there's some good stuff in this PR, such as the support for explicitly marking a file as documentation, changing the Unix file mode,... .

Do you plan to complete the PR? git rebase can be messy; an alternative may be to cherry-pick the commits you want to preserve in a new branch.

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.

3 participants