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

Add CompileStep APIs to Build.Module. #14731

Closed
wants to merge 39 commits into from

Conversation

AdamGoertz
Copy link
Contributor

@AdamGoertz AdamGoertz commented Feb 26, 2023

Closes #14719. Adding these APIs to Build.Module allows for needed include paths and libraries to be declared at the module's creation site (in the dependency) rather than in the dependee's code.

var mod = b.addModule(.{
    .name = "mymodule",
    .source_file = "...",
});
mod.addIncludePath(include_dir);

I'm not sure of the full set of CompileStep functions that need to be ported to Build.Module, but I made a start on the most obvious ones.

Functions added to Build.Module so far:

  • addIncludePath
  • addConfigHeader
  • addLibraryPath
  • addLibraryPathDirectorySource
  • addObject
  • addAssemblyFile
  • addAssemblyFileSource
  • linkLibC
  • linkLibCpp
  • linkLibrary
  • linkSystemLibrary
  • linkSystemLibraryNeeded
  • linkSystemLibraryWeak
  • linkSystemLibraryName
  • linkSystemLibraryNeededName
  • linkSystemLibraryWeakName
  • installHeader
  • installConfigHeader
  • installHeadersDirectory
  • installHeadersDirectoryOptions
  • installLibraryHeaders
  • linkFramework
  • linkFrameworkNeeded
  • linkFrameworkWeak
  • addSystemIncludePath
  • addRPath
  • addRPathDirectorySource
  • addFrameworkPath
  • addFrameworkPathDirectorySource
  • linkSystemLibraryPkgConfigOnly
  • linkSystemLibraryNeededPkgConfigOnly
  • dependsOnSystemLibrary
  • defineCMacro
  • defineCMacroRaw
  • addCSourceFileSource
  • addCSourceFile
  • addCSourceFiles
  • addOptions (see More consistent way to add build options modules #14979)

Functions added to Module so far:
* `addIncludePath`
* `addConfigHeader
* `addLibraryPath`
* `linkLibC`
* `linkLibCpp`
* `linkLibrary`
* `linkSystemLibrary`
@AdamGoertz AdamGoertz marked this pull request as draft February 26, 2023 00:48
@hryx
Copy link
Contributor

hryx commented Feb 26, 2023

Great, looking forward to having this! Thanks for having addModule() return the module as well.

From the compiler meeting this week, it sounds like Module should have installHeader() and friends too.

lib/std/Build.zig Outdated Show resolved Hide resolved
lib/std/Build.zig Outdated Show resolved Hide resolved
lib/std/Build.zig Outdated Show resolved Hide resolved
lib/std/Build/CompileStep.zig Outdated Show resolved Hide resolved
lib/std/Build.zig Outdated Show resolved Hide resolved
These implementations are not yet tested. Newly added functions are:
* `linkSystemLibraryNeeded`
* `linkSystemLibraryWeak`
* `linkSystemLibraryName`
* `linkSystemLibraryNeededName`
* `linkSystemLibraryWeakName`
* `installHeader`
* `installConfigHeader`
* `installHeadersDirectory`
* `installHeadersDirectoryOptions`
* `installLibraryHeaders`
Adds tests for:
* `addIncludePath`
* `linkLibrary`
* `linkSystemLibrary` + `addLibraryPath`
* `installHeader`
Adds:
* `linkFramework`
* `linkFrameworkNeeded`
* `linkFrameworkWeak`
Adds
* `addSystemIncludePath`
* `addRPath`
* `addFrameworkPath`
* `linkSystemLibraryPkgConfigOnly`
* `linkSystemLibraryNeededPkgConfigOnly`
@andrewrk
Copy link
Member

andrewrk commented Mar 1, 2023

Is this ready for review?

@AdamGoertz AdamGoertz marked this pull request as ready for review March 1, 2023 23:13
@AdamGoertz
Copy link
Contributor Author

Is this ready for review?

Yes, I think so. Soliciting some more reviews was on my to-do list for this evening. Thanks for the reminder

@AdamGoertz AdamGoertz requested a review from mlugg March 1, 2023 23:14
@AdamGoertz AdamGoertz changed the title Draft: Add CompileStep APIs to Build.Module. Add CompileStep APIs to Build.Module. Mar 2, 2023
Copy link
Contributor

@ianprime0509 ianprime0509 left a comment

Choose a reason for hiding this comment

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

This is really useful, thank you! I noticed a couple small issues when I tried to use this in one of my projects.

lib/std/Build.zig Outdated Show resolved Hide resolved
lib/std/Build.zig Outdated Show resolved Hide resolved
@andrewrk
Copy link
Member

I will take this from here - thank you

@AdamGoertz
Copy link
Contributor Author

Sounds good. I've been on work travel for most of the past few weeks, so I haven't had a chance to resolve conflicts. I'm back now, so just let me know if there's anything else I can do on this. Otherwise I'll leave it to you.

Copy link
Member

@andrewrk andrewrk left a comment

Choose a reason for hiding this comment

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

Alright, I took a good look at this, and I feel confident about the path forward. It's going to take some reworking and some more ruthless API breakage, but the end result will be an overall simpler, more coherent system, that is easier to learn and reason about, especially when using packages as third-party dependencies.

All these APIs that are redundant between Module and CompileStep - surprise - it's actually because CompileStep is composed of Modules.

This comment from @hryx is revealing:

Note: I'm not sure which APIs should be copied from CompileStep. All of them? Just ones related to installing headers?

If you think of CompileStep as being redefined in terms of a main Module, then all those APIs move over into the Module namespace instead of a subset of them being arbitrarily copied.

Consider the fact that one also would potentially want to add C source files to be part of a Module exposed from a package. A big chunk of CompileStep should move into Module. Care will need to be taken to design the new API to not be confusing or awkward compared to status quo, which suffers from but also benefits from a more naive model of things.

Let me give an example:

// old
const exe = b.addExecutable(.{
    .name = "aoeu",
});
exe.addIncludePath(.{ .path = "blah" });

// new
const exe = b.addExecutable(.{
    .name = "aoeu",
});
exe.main_module.addIncludePath(.{ .path = "blah" });

Note that currently, Module has a non-optional root source file for the zig source file. This enhancement would make it possible to have a Module that did not have any zig source code associated with it (perhaps there would be C source files only, for example).

This may require a little bit of enhancement to zig's CLI because it will make it possible to represent, for example, one Module having a C source file with some include paths, and another Module having a different C source file with different include paths. This is not currently represent in the CLI, but it should be. Organizing the build system API in a coherent way is making this limitation more obvious. cc @mlugg who worked on this recently.

So, if you want to push forward on this, here are the steps:

  1. Rebase against master branch
  2. Let's go ahead and extract Module from Build.zig into Build/Module.zig
  3. Move all logic from CompileStep into Module that makes sense to go there, while introducing a "main module" field to CompileStep.
  4. The make() function of CompileStep will need to be reworked to traverse over the modules and pass that information along to the CLI
  5. The CLI needs to support CLI flags per module.
  6. For both CLI and build.zig, perhaps some (temporary? permanent?) helper API can exist which supports the previous API and simply forwards the args/flags to the main module.

I can understand if this is too big of a task for someone who was trying to just address one well-defined issue. So please let me know and I will be happy to close this PR and start working on the changes myself.

lib/std/Build.zig Outdated Show resolved Hide resolved
lib/std/Build.zig Outdated Show resolved Hide resolved
lib/std/Build.zig Outdated Show resolved Hide resolved
lib/std/Build/CompileStep.zig Outdated Show resolved Hide resolved
test/standalone/emit_asm_and_bin/.gitignore Outdated Show resolved Hide resolved
@mlugg
Copy link
Member

mlugg commented Apr 21, 2023

Hi! Andrew's comments here tie into some changes I was planning on doing to the CLI, where you pass a main module instead of a main file - I was actually waiting on a few changes, this PR included, just to avoid potential nasty merges.

@andrewrk, what're the chances of intern-pool-2 being looked at soon? One of the genuinely very confusing things when working on said CLI rework was the current overloading of the term "module" in the compiler source, since it turned into a semi-major rework of module lifetimes; so if it's possible to get on with renaming Module any time soon that would be fantastic. But if that'll still be a wait, I can push ahead with the CLI changes anyway and just hope I don't go insane :p

These changes will not last, as I don't think they'll work with deeper dependency trees, but for now they'll do.
Adding a `Step` field to Build/Module.zig allow us to more easily track
dependencies. CompileStep can now depend on main_module.step instead of
all of the dependencies of main_module.
This may need to move back to Moulde eventually, but the goal is the handle the easy + common cases first
In the interest of keeping things simple, these can stay in CompileStep for now. They may need to be moved to Module later
Bootstrap now builds, but stage3 fails because we're not yet
passing the main_module correctly, so the compilation does not have
a root source file yet.
@edyu
Copy link
Contributor

edyu commented Jun 28, 2023

#16172 might be fixed by this?

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.

std.Build.Module should include APIs from CompileStep
6 participants