feat(compiler)!: Out of source builds#2384
feat(compiler)!: Out of source builds#2384ospencer wants to merge 5 commits intooscar/gc-rebasedfrom
Conversation
ospencer
requested review from
alex-snezhko,
marcusroberts,
peblair,
phated and
spotandjake
as code owners
spotandjake
left a comment
spotandjake
left a comment
There was a problem hiding this comment.
This looks great to me, I did an initial review and has a few thoughts, I'll get you a more in depth one tomorrow or later this week.
| const success = exec.grainc(file, options, program); | ||
| if (success) { | ||
| const outFile = options.o ?? file.replace(/\.gr$/, ".wasm"); | ||
| const outFile = options.o ?? defaultWasmLocation(file, options); |
There was a problem hiding this comment.
How would we feel about setting options.o by default in the cli to the root directory so the wasm file is still copied out by our cli.
I think this provides a nicer user experience with the js api having the wasm file just be there.
There was a problem hiding this comment.
I think it's better to not have the file be in the source directory by default. grain file.gr will still run the file for you, and it's not hard to find. Plus, if you want it somewhere else you can always pass -o.
| // Add global options to command | ||
| cmd.forwardOption( | ||
| "-I, --include-dirs <dirs>", | ||
| "add additional dependency include directories", |
There was a problem hiding this comment.
| "add additional included object file directories", |
Is it useful for us to actually expose this through our cli, should we just remove the flag here and if someone needs that they can invoke grainc directly?
My main worry is that people are going to get confused about what this is / does
There was a problem hiding this comment.
I could see an argument for that.
| `Ok(); | ||
| }; | ||
|
|
||
| /** Converter which checks that the given output filename is valid */ |
There was a problem hiding this comment.
Why did we remove this comment? The function still looks todo the same thing?
|
|
||
| type origin = | ||
| | Project | ||
| | Stdlib |
There was a problem hiding this comment.
Is there a reason we treat the stdlib separately here? Why not consider it a Project?
There was a problem hiding this comment.
Project is not a project, it's this project. (A better argument would be why the stdlib isn't just a Library, but that's because the module resolution happens differently because we don't prefix all the stdlib imports with stdlib.)
| let chop_suffix = Filename.chop_suffix; | ||
|
|
||
| let first_segment = path => | ||
| switch (String.index_opt(path, '/')) { |
There was a problem hiding this comment.
shouldn't we be checking for both / and \ or normalizing first?
There was a problem hiding this comment.
Does this pr allow us to drop the pkg.js file in the cli?
It only exists to map the build outputs to a target directory but we are doing that inside the compiler now?
(Also just remembered that we are going to need to update the playground to handle the out of source stuff before we release this version if we merge this).
2 participants
This PR moves all produced build artifacts to a target directory. By default, it's called
target.The folder structure is as follows:
The
builddirectory contains all artifacts for the current project and is anchored by a project root, which can optionally be configured via--project-root. Thedepsfolder contains all build artifacts for external libraries. Besides the stdlib, additional libraries can be included with-L. This differs from-I, which is now only for including directories of object files (relevant for build systems). By default, the wasm artifact is now placed attarget/[profile]/artifact.wasm.Compilation behavior remains the same, but there is now much better support for build systems that want to compile libraries separately and include them later with
-I.Typed filepaths from the
Fplibrary were used as much as possible through the refactor ofmodule_resolution.Closes #468