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

koch: introduce builder #133

Draft
wants to merge 2 commits into
base: devel
Choose a base branch
from
Draft

koch: introduce builder #133

wants to merge 2 commits into from

Conversation

alaviss
Copy link
Contributor

@alaviss alaviss commented Dec 27, 2021

This is the first implementation of #123

Builder is a new component enabling build actions to be declared as
opposed to be executed imperatively. This allow for side-effect less
declaration of build pipeline and enable them to be parallelized.

This library is heavily influenced by Meson with a touch of zig build.

This first implementation is incomplete and have a few issues, posted here so everyone can keep track and make suggestions:

  • Docs might be out-of-date
  • Cannot do bootstrapping
  • No CLI interface yet
  • Does not silence nim compiler calls (which makes it spammy in parallel
    runs)
  • Artifacts are still being placed outside of the build directory
  • Target description is not supported yet
  • koch integration is kind of hacky
  • No tests
  • Plenty of dead code
  • No clean function
  • No install function

This is the first implementation of
#123

Builder is a new component enabling build actions to be declared as
opposed to be executed imperatively. This allow for side-effect less
declaration of build pipeline and enable them to be parallelized.

This first implementation is incomplete and have a few issues:

- Docs might be out-of-date
- Cannot do bootstrapping
- No CLI interface yet
- Does not silence nim compiler calls (which makes it spammy in parallel
  runs)
- Artifacts are still being placed outside of the build directory
- Target description is not supported yet
- koch integration is kind of hacky
Comment on lines +207 to +233
func sortTargets(b: var Builder): seq[Target] =
## Perform a topological sort on builders' targets.
let sortedOrder = b.topologicalOrder

var
newNames: typeof(b.names)
newRecipes: typeof(b.recipes)
newDependencies: typeof(b.dependencies)

for idx, target in sortedOrder.pairs:
# Insert to new target seq according to the sorted order
newRecipes.add b.recipes[target]
newNames.add b.names[target]

# If the target has any dependencies
let deps = b.dependencies.getOrDefault(target)
if deps != {}:
for newDep in Target(0) ..< target:
let dep = sortedOrder[newDep]
# If dep was a dependency of the target
if dep in deps:
# Re-link it to the new ID
newDependencies[target].incl newDep

b.names = newNames
b.recipes = newRecipes
b.dependencies = newDependencies
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dead code

Copy link
Collaborator

@saem saem left a comment

Choose a reason for hiding this comment

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

API looks really good, though I think addRun might benefit from a tweak.

a: Operand ## First operand
b: Operand ## Second operand

Builder* = object
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would lead with this, since type sections rearrange -- this gives a nice map of the concepts/data.

tools/koch/builder.nim Outdated Show resolved Hide resolved
tools/koch/builder.nim Outdated Show resolved Hide resolved
tools/koch/builder.nim Outdated Show resolved Hide resolved
tools/koch/builder.nim Outdated Show resolved Hide resolved
import std/[os, osproc, parseopt, sequtils, setutils, tables]

type
Target* = distinct uint16
Copy link
Collaborator

Choose a reason for hiding this comment

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

General remark for DOD: Part of me would consider reserving Target and such names for 'view objects' that provide an API over the internals; but the builder isn't really a library right now so I wouldn't worry about it yet.

tools/koch/builder.nim Outdated Show resolved Hide resolved
b.recipes = newRecipes
b.dependencies = newDependencies

func name*(b: Builder, target: Target): string =
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is where it might be nice to have Target be TargetId and Target be:

type
  Target = object
    builder: Builder
    id: TargetId

at least from the outside people are dealing with a friendlier concept of a holistic target, while internally the memory layout and data are nice and orderly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will be at one point, but having builder as a ref is not something I'd like now due to how easy it is to hide mutations.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well it could only happen inside module defined procs and if you wanted extra safety the builder could be a distinct to reduce the surface further.

@@ -243,6 +240,15 @@ proc buildTools(args: string = "") =
"--opt:speed --stacktrace -d:debug --stacktraceMsgs -d:nimCompilerStacktraceHints --excessiveStackTrace:off " & defineSourceMetadata() & " " & args,
outputName = "nim_dbg")

proc addTools(b: var Builder) =
let tools = [
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe a b.buildGroup call that takes a name and a bunch of addxxx calls, returning the name as an alias?

let compilerManifest = nimSource / "compiler" / "installer.ini"

let buildScript: Target = b.addRun("installscripts", niminst, @defaultArgs & @["scripts", compilerManifest])
discard b.addRun("archive", niminst, @defaultArgs & @["--format:tar.xz", "archive", compilerManifest], dependsOn = {buildScript})
Copy link
Collaborator

Choose a reason for hiding this comment

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

few cases where I think discardable makes sense, I don't think it catches errors here; because the dependency passing will force that.

## Returns the path to the nimcache of the executable
b.buildDir / "nimcache" / b.name(exe)

func addRun*(b: var Builder, name: string, exe: Executable,
Copy link
Collaborator

Choose a reason for hiding this comment

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

To get transactional behavior:

  1. Immediately make a copy of b
  2. Wrap the current body in a try
  3. manipulate the copy instead of b in the try
  4. in the last line of the try copy back the changes over from the copy into b
  5. profit ... and by that I mean you probably want to make that a template

@haxscramper haxscramper added the tool Improvements to non-compiler tooling label Nov 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tool Improvements to non-compiler tooling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants