-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
std.Build: Deprecate Step.Compile
APIs that mutate the root module
#22587
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
Conversation
@mlugg would you mind reviewing this (considering #20388/#18752)? Also @BratishkaErik. |
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.
Sounds good, let's make this depend on #22822 yeah?
@andrewrk Updating these APIs to make use of |
a8cfdb5
to
8d7acbe
Compare
8d7acbe
to
137094d
Compare
This did not make the 0.14 cutoff. I've removed references to version numbers from the doc comments so this can be merged whenever you feel is appropriate. |
28049d3
to
cc1ab4e
Compare
42a65c4
to
04c283c
Compare
This looks good to me. Note that reaching through to One last request, sorry. Would it be possible to add If you can amend the first commit with that, I'll then be happy to press the big green button -- you can give me a ping on Zulip to make sure I don't miss it. |
Not only are `Step.Compile` methods like `linkLibC()` redundant because `Module` exposes the same APIs, it also might not be immediately obvious to users that these methods modify the underlying root module, which can be a footgun and lead to unintended results if the module is exported to package consumers or shared by multiple compile steps. Using `compile.root_module.link_libc = true` makes it more clear to users which of the compile step and the module owns which options.
04c283c
to
154bd2f
Compare
I agree that it's the better and more hygienic style to construct and configure the module separately since modifying |
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.
This looks great, thanks very much. You can add a mention to the release notes of the fact that directly using exe.root_module
(etc) is discouraged if you would like to, but it's not that important, so don't worry too much.
Not only are
Step.Compile
methods likelinkLibC()
redundant becauseModule
exposes the same APIs, it also might not be immediately obvious to users that these methods modify the underlying root module, which can be a footgun and lead to unintended results if the module is exported to package consumers or shared by multiple compile steps. For example, I personally almost ran in to this:Using
compile.root_module.link_libc = true
orcompile.root_module.linkLibrary(lib)
makes it more clear to users which of the compile step and the module owns which options.Suggested release notes (if you think deprecation notices are relevant):
Deprecated
std.Build.Step.Compile
APIsThe following
std.Build.Step.Compile
methods have been deprecated in favor of theirstd.Build.Module
equivalents and are slated for removal during the next release cycle:addAfterIncludePath
addAssemblyFile
addCSourceFile
addCSourceFiles
addConfigHeader
addEmbedPath
addFrameworkPath
addIncludePath
addLibraryPath
addObject
addObjectFile
addRPath
addSystemFrameworkPath
addSystemIncludePath
addWin32ResourceFile
linkFramework
linkLibC
linkLibCpp
linkLibrary
linkSystemLibrary
linkSystemLibrary2
Upgrade guide: