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

[dotnet] Remove extra/unnecessary binaries from git #15063

Merged
merged 5 commits into from
Jan 17, 2025

Conversation

nvborisenko
Copy link
Member

@nvborisenko nvborisenko commented Jan 12, 2025

User description

Description

In my understanding tools should not be a part of any source control system.

Motivation and Context

Keeping only source code, not binaries.

selenium/third_party/dotnet folder:

  • Before: 49.0 MB (51,412,992 bytes)
  • After: 104 KB (106,496 bytes)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

PR Type

Enhancement


Description

  • Removed unnecessary binaries and tools from the repository to maintain a clean source control system.

  • Deleted multiple files under third_party/dotnet, including binaries, scripts, and configuration files.

  • Ensured that only source code remains in the repository, aligning with best practices for source control.


Changes walkthrough 📝

Relevant files

💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

Copy link
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ No major issues detected

Copy link
Contributor

PR Code Suggestions ✨

No code suggestions found for the PR.

@nvborisenko nvborisenko requested a review from shs96c January 12, 2025 16:27
@nvborisenko
Copy link
Member Author

I should test it on the clean system (fresh clone, no nuget cache)

@RenderMichael
Copy link
Contributor

RenderMichael commented Jan 16, 2025

Some of these looks like dead code. The ZipStorer is no longer used as on Selenium 4:

selenium/dotnet/CHANGELOG

Lines 967 to 969 in efe5a34

* Removed custom zip archive code. Instead of using the custom ZipStorer
class, we can now use the built-in .NET zip archive handling classes. This
is possible because we no longer support .NET versions less than 4.5.

ILMerge looks like it is used in places:

merged_assembly = rule(
implementation = _merged_assembly_impl,
attrs = {
"src_assembly": attr.label(),
"deps": attr.label_list(),
"out": attr.string(default = ""),
"keyfile": attr.label(allow_single_file = True),
"target_framework": attr.string(mandatory = True),
"merge_tool": attr.label(
executable = True,
cfg = "exec",
default = Label("//third_party/dotnet/ilmerge:ilmerge.exe"),
allow_single_file = True,
),
},
toolchains = ["//third_party/dotnet/ilmerge:toolchain_type"],
)

native.register_toolchains("//third_party/dotnet/ilmerge:all")

The NuGet dependency may have some significance:

<?xml version="1.0" encoding="utf-8"?>
<configuration>
<packageSources>
<add key="Selenium Project Packages" value="../third_party/dotnet/nuget/packages" />
</packageSources>
</configuration>

No pings grepping for ilrepack, NUnit console, or stylecop. Those three and ZipStorer should be free for removal. The other ones may need to be untangled or left in there.

@RenderMichael
Copy link
Contributor

RenderMichael commented Jan 16, 2025

I found more places that use the third_party/dotnet/nuget library. I think this one may actually be useful somehow, but I'm not sure if those rules are ever run anymore or if they are legacy.

nuget_push = rule(
implementation = _nuget_push_impl,
attrs = {
"src": attr.label(
allow_single_file = True,
),
"deps": attr.label_list(),
"package_repository_url": attr.string(
default = "https://nuget.org",
),
"api_key": attr.label(default = ":nuget-api-key"),
"nuget_exe": attr.label(
executable = True,
cfg = "exec",
default = Label("//third_party/dotnet/nuget:nuget.exe"),
allow_single_file = True,
),
},
)

import_nuget_package(
name = "json.net",
file = "third_party/dotnet/nuget/packages/newtonsoft.json.13.0.1.nupkg",
sha256 = "2b6b52556e27e1b7913f33eedeb95568110c746bd64afff74357f1683878323a",
)
import_nuget_package(
name = "moq",
file = "third_party/dotnet/nuget/packages/moq.4.12.0.nupkg",
sha256 = "339bbb71107e137a753a89c6b74adb5d9072f0916cf8f19f48b30ae29c41f434",
)
# Moq depends on Castle.Core
import_nuget_package(
name = "castle.core",
file = "third_party/dotnet/nuget/packages/castle.core.4.4.0.nupkg",
sha256 = "ee12c10079c1f9daebdb2538c37a34e5e317d800f2feb5cddd744f067d5dec66",
)
import_nuget_package(
name = "benderproxy",
file = "third_party/dotnet/nuget/packages/benderproxy.1.0.0.nupkg",
sha256 = "fd536dc97eb71268392173e7c4c0699795a31f6843470134ee068ade1be4b57d",
)
import_nuget_package(
name = "nunit",
file = "third_party/dotnet/nuget/packages/nunit.3.12.0.nupkg",
#sha256 = "056eec5d3d8b2a93f7ca5b026d34d9d5fe8c835b11e322faf1a2551da25c4e70",
)
import_nuget_package(
name = "handlebars",
file = "third_party/dotnet/nuget/packages/handlebars.net.1.11.5.nupkg",
sha256 = "5771ef7dddbf0024e25456f26ffaaf75023847a8c0f5b8be1d832c1ef2a41c96",
)
# Handlebars.Net depends on Microsoft.CSharp
import_nuget_package(
name = "csharp",
file = "third_party/dotnet/nuget/packages/microsoft.csharp.4.7.0.nupkg",
sha256 = "127927bf646c145ebc9443ddadfe4cf81a55d641e82d3551029294c2e93fa63d",
)
import_nuget_package(
name = "humanizer",
file = "third_party/dotnet/nuget/packages/humanizer.core.2.8.26.nupkg",
sha256 = "555b42765a0adefcfd6cfab486a1da195716bb72066ed26ac098e8ea45681ded",
)
import_nuget_package(
name = "dependencyinjection",
file = "third_party/dotnet/nuget/packages/microsoft.extensions.dependencyinjection.3.1.9.nupkg",
sha256 = "6b4ddfc1c8d83139e8f1b8bd6cc0b2413b85362622d4ae547fb1b4edf897d2c5",
)
# Microsoft.Extensions.DependencyInjection depends on Microsoft.Extensions.DependencyInjection.Abstractions
import_nuget_package(
name = "dependencyinjectionabstractions",
file = "third_party/dotnet/nuget/packages/microsoft.extensions.dependencyinjection.abstractions.3.1.9.nupkg",
sha256 = "664b74ebd587279e3697e2db79e67199a75da1089479813d6ddca1e0c379f6d0",
)
import_nuget_package(
name = "commandlineparser",
file = "third_party/dotnet/nuget/packages/commandlineparser.2.8.0.nupkg",
sha256 = "6b6568155442c2a4fb2ca4442f245bf401c11078ad212f4b9967894da3ef62d4",
)

@nvborisenko
Copy link
Member Author

I deleted everything I could:

  • local nuget packages (I believe we don't keep dependencies in git)
  • nuget.exe file (it is not cross-platform, and we don't use it)
  • ILMerge (we want to forget it)

Verified on clean build environment.

Copy link
Contributor

@RenderMichael RenderMichael left a comment

Choose a reason for hiding this comment

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

A great simplification!

@nvborisenko
Copy link
Member Author

Simon verbally approved it if fresh build works. I t works, so cross finger.

@nvborisenko nvborisenko merged commit 1e86898 into SeleniumHQ:trunk Jan 17, 2025
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants