-
Notifications
You must be signed in to change notification settings - Fork 369
Improved C# support #1258
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
base: main
Are you sure you want to change the base?
Improved C# support #1258
Changes from 17 commits
dab0f7a
c2e8367
55f33ef
3abbbcd
7754471
4b237e0
54d4c12
8ab8c8f
4060394
d58d501
69df7d5
e83525e
215f649
fd7b704
7a745b0
f5496e6
9864ad3
af57649
9c8007a
567466f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| # Copyright (c) Meta Platforms, Inc. and affiliates. | ||
| # | ||
| # This source code is dual-licensed under either the MIT license found in the | ||
| # LICENSE-MIT file in the root directory of this source tree or the Apache | ||
| # License, Version 2.0 found in the LICENSE-APACHE file in the root directory | ||
| # of this source tree. You may select, at your option, one of the | ||
| # above-listed licenses. | ||
|
|
||
| # TODO(cjhopman): This was generated by scripts/hacks/rules_shim_with_docs.py, | ||
| # but should be manually edited going forward. There may be some errors in | ||
| # the generated docs, and so those should be verified to be accurate and | ||
| # well-formatted (and then delete this TODO) | ||
|
|
||
| FrameworkVersion = ["net35", "net40", "net45", "net46"] |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,8 +12,33 @@ | |
| # well-formatted (and then delete this TODO) | ||
|
|
||
| load(":common.bzl", "buck", "prelude_rule") | ||
| load(":dotnet_common.bzl", "FrameworkVersion") | ||
|
|
||
| FrameworkVersion = ["net35", "net40", "net45", "net46"] | ||
| _CSHARP_LIBRARY_OR_EXE_ATTRIBUTES = { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All of the comments below are almost all just improvements to what was already pre-existing. While we can fix it, we should. If you're not certain about this, when we go to merge it I can make edits, too.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I've fixed all the issues except for the |
||
| "srcs": attrs.list(attrs.source(), default = [], doc = """ | ||
| The set of C# source files to be compiled, and assembled by this rule. | ||
| Each element must a string specifying a source file. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "must a string specifying a source file" is wrong. You're missing "be", and it isn't a string specifying a source file in all instances... attrs.source entries can be target names, too. Imagine you have a script that generates a source file from some typing information (like an *.idl file). This would be a reference to an output of that target. https://buck2.build/docs/api/build/attrs/#source
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed, basically copied the sentences from that documentation link |
||
| """), | ||
| "resources": attrs.dict(key = attrs.string(), value = attrs.source(), sorted = False, default = {}, doc = """ | ||
| Resources that should be embedded within the built DLL. The format | ||
| is the name of the resource once mapped into the DLL as the key, and | ||
| the value being the resource that should be merged. This allows | ||
| non-unique keys to be identified quickly. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What does "once mapped" mean? "This allows non-unique keys to be identified quickly"... this is pertinent to the operation of the end binary rule, but it doesn't mean much to the user. You might instead say: "The X rule merges the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've removed the resources attribute since it has not been implemented yet. |
||
| """), | ||
| "framework_ver": attrs.enum(FrameworkVersion, doc = """ | ||
| The version of the .Net framework that this library targets. This is | ||
| one of 'net35', 'net40', 'net45' and 'net46'. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You don't want to have to keep this list up to date. Either generate the string here from
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've generated the list from |
||
| """), | ||
| "deps": attrs.list(attrs.one_of(attrs.dep(), attrs.string()), default = [], doc = """ | ||
| The set of targets or system-provided assemblies to rely on. Any | ||
| values that are targets must be either csharp\\_library or `prebuilt_dotnet_library` | ||
| instances. | ||
| """), | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "to rely on" => "this target depends on." What does it mean for this to be an If you depend on a particular provider being in here, your
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The attrs.string is for "system-provided assemblies" like System.IO.dll |
||
| "compiler_flags": attrs.list(attrs.string(), default = [], doc = """ | ||
| The set of additional compiler flags to pass to the compiler. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be an
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
| """), | ||
| "add_hermetic_arguments": attrs.bool(default = True), | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Needs docs. What are the "hermetic arguments", or why would I want this?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And this one it looks like you added, so I do want you to write the docs for it.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've added documentation for this argument |
||
| } | ||
|
|
||
| csharp_library = prelude_rule( | ||
| name = "csharp_library", | ||
|
|
@@ -22,8 +47,6 @@ csharp_library = prelude_rule( | |
| and dependencies by invoking csc. | ||
| """, | ||
| examples = """ | ||
| For more examples, check out our [integration tests](https://github.com/facebook/buck/tree/dev/test/com/facebook/buck/rust/testdata/). | ||
|
|
||
| ``` | ||
| csharp_library( | ||
| name = 'simple', | ||
|
|
@@ -57,57 +80,64 @@ csharp_library = prelude_rule( | |
| The output name of the dll. This allows you to specify the name of | ||
| the dll exactly. When this is not set, the dll will be named after | ||
| the short name of the target. | ||
| """, | ||
| ), | ||
| "srcs": attrs.list( | ||
| attrs.source(), | ||
| default = [], | ||
| doc = """ | ||
| The collection of source files to compile. | ||
| """, | ||
| ), | ||
| "resources": attrs.dict( | ||
| key = attrs.string(), | ||
| value = attrs.source(), | ||
| sorted = False, | ||
| default = {}, | ||
| doc = """ | ||
| Resources that should be embedded within the built DLL. The format | ||
| is the name of the resource once mapped into the DLL as the key, and | ||
| the value being the resource that should be merged. This allows | ||
| non-unique keys to be identified quickly. | ||
| """, | ||
| ), | ||
| "framework_ver": attrs.enum( | ||
| FrameworkVersion, | ||
| doc = """ | ||
| The version of the .Net framework that this library targets. This is | ||
| one of 'net35', 'net40', 'net45' and 'net46'. | ||
| """, | ||
| ), | ||
| "deps": attrs.list( | ||
| attrs.one_of(attrs.dep(), attrs.string()), | ||
| default = [], | ||
| doc = """ | ||
| The set of targets or system-provided assemblies to rely on. Any | ||
| values that are targets must be either csharp\\_library or `prebuilt_dotnet_library` | ||
| instances. | ||
| """, | ||
| ), | ||
| "compiler_flags": attrs.list( | ||
| attrs.string(), | ||
| default = [], | ||
| doc = """ | ||
| The set of additional compiler flags to pass to the compiler. | ||
| """, | ||
| ), | ||
| """), | ||
| } | ||
| | _CSHARP_LIBRARY_OR_EXE_ATTRIBUTES | ||
| | buck.licenses_arg() | ||
| | buck.labels_arg() | ||
| | buck.contacts_arg() | ||
| ), | ||
| ) | ||
|
|
||
| csharp_binary = prelude_rule( | ||
| name = "csharp_binary", | ||
| docs = """ | ||
| A csharp\\_binary() rule builds a .Net library from the supplied set of C# source files | ||
| and dependencies by invoking csc. | ||
| """, | ||
| examples = """ | ||
| ``` | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you can do "```python" here to get python syntax highlighting in the generated docs
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
|
|
||
| csharp_binary( | ||
| name = 'simple', | ||
| exe_name = 'Cake.exe', | ||
| framework_ver = 'net46', | ||
| srcs = [ | ||
| 'Hello.cs', | ||
| ], | ||
| resources = { | ||
| 'greeting.txt': '//some:target', | ||
| }, | ||
| deps=[ | ||
| ':other', | ||
| 'System.dll', | ||
| ], | ||
| ) | ||
|
|
||
| prebuilt_dotnet_library( | ||
| name = 'other', | ||
| assembly = 'other-1.0.dll', | ||
| ) | ||
|
|
||
| ``` | ||
| """, | ||
| further = None, | ||
| attrs = ( | ||
| # @unsorted-dict-items | ||
| { | ||
| "exe_name": attrs.string(default = "", doc = """ | ||
| The output name of the dll. This allows you to specify the name of | ||
| the dll exactly. When this is not set, the dll will be named after | ||
| the short name of the target. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "the dll filename will be the target's name attribute, suffixed with...X". I think X depends on whether it's an exe or dll? Or will this always be dll?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for noticing, I had just copied that from csharp_library, I've fixed it now by replacing dll with executable. |
||
| """), | ||
| } | | ||
| _CSHARP_LIBRARY_OR_EXE_ATTRIBUTES | | ||
| buck.licenses_arg() | | ||
| buck.labels_arg() | | ||
| buck.contacts_arg() | ||
| ), | ||
| ) | ||
|
|
||
| prebuilt_dotnet_library = prelude_rule( | ||
| name = "prebuilt_dotnet_library", | ||
| docs = """ | ||
|
|
@@ -152,5 +182,6 @@ prebuilt_dotnet_library = prelude_rule( | |
|
|
||
| dotnet_rules = struct( | ||
| csharp_library = csharp_library, | ||
| csharp_binary = csharp_binary, | ||
| prebuilt_dotnet_library = prebuilt_dotnet_library, | ||
| ) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a hazard here that can't be directly fixed. In a very large project, you may have MANY entries here, and the size of this can blow up. You do have to enumerate these at some point and create the symlinks, though.
Another approach is to farm out the symlink creation to a script/tool/whatever that would create all the symlinks for you, and instead of calling
child.traverse()here, you'd give that script the result of a "projection" on the transitive set. In your implementation,new_runtime_fileswill be the size of the graph represented by the TSet. In the other approach, the projection is just a refernce to the TSet root that says "run the X projection", and the TSet is only walked when the action that runs the script is run. Thepython_binaryrules do this sort of thing to avoid building these (huge) lists in prelude/python/python.bzl, in theargs_projections. These are later used to build manifest files or symlink trees (like you're doing here) with the help of external commands that take the list of arguments as inputs in some form. The python rules used.traverse()at one time, consuming single-digit gigabytes of memory for eachpython_binarytarget.This isn't functionally wrong, but anywhere you can avoid copying the contents of a TSet is an opportunity to not waste memory. Because artifacts need to be declared, doing this work with
.symlink_filehas to do this, I think. I don't believe there's a way currently to say "build up a bunch of symlinks here by walking this TSet", hence the external tools that take the list of symlinks as arguments in some form and create them.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I figured the optimal way would be to avoid traverse but I didn't see a way of doing that. I guess using a separate tool would work but it seems harder to maintain rather than using Starlark's native symlink_file.
Could we merge this like this and if people start raising issues with this taking a lot of memory we can implement this external tool improvement? With any luck by that time there will be a better way of doing this.