sdk icon indicating copy to clipboard operation
sdk copied to clipboard

🪝 [breaking change] Reserve top-level `hook/` directory 🪝

Open dcharkes opened this issue 1 year ago • 47 comments
trafficstars

Change intent

(edit: Updated 2024-01-24 to reflect use of the hook/ toplevel directory.)

A package toplevel hook/ directory will be reserved for scripts run by the Dart and Flutter SDK at specific lifecycle events with a specific commandline API.

Examples:

  • hook/build.dart will be run before kernel compilation of Dart packages.
  • hook/link.dart will be run after kernel compilation of Dart packages, before making an application bundle.

If these scripts exist, they will be invoked by the Dart and Flutter build systems.

If these scripts exist, they must implement the commandline API.

For more context:

  • https://github.com/dart-lang/sdk/issues/50565

For more

Justification

Other languages standardize on a top level build script that builds code in other languages.

Most notably Rust defines a top level build.rs in packages to build native non-Rust code.

  • https://doc.rust-lang.org/cargo/reference/build-scripts.html

Reasoning for using hook/build.dart and hook/link.dart in Dart.

  • A Dart package public surface is the lib/ and bin/ directory. But build scripts should not be part of the programmatic or CLI API of a package.
  • A package private scripts reside in tool/. But build scripts should be run from the context outside that package itself.
  • The build/ directory is already reserved in Flutter. So nesting the build script inside the build/ directory doesn't work.

Alternatives considered

  • toplevel build.dart and link.dart. Rejected because we might want more scripts.
  • toplevel build_assets.dart and link_assets.dart. Rejected because of longer names.
  • https://github.com/dart-lang/sdk/issues/54111

Impact

Anyone using a top level hook/ directory will need to migrate to a different file name.

We are aware of one use:

  • https://github.com/epam-cross-platform-lab/swagger-dart-code-generator/issues/681

Mitigation

Migrate existing uses of hook/ to tool/, which is probably what those scripts are.

Action items after accepting proposal:

  • [ ] Update https://dart.dev/tools/pub/package-layout to include hook/
  • [ ] https://github.com/dart-lang/native/issues/823
  • [ ] https://github.com/flutter/flutter/issues/138727

dcharkes avatar Dec 13 '23 09:12 dcharkes

Ready for another round of bike shedding. @mit-mit @mraleph @mkustermann @mosuem @devoncarew @HosseinYousefi @jonasfj and who else wants to chime in.

dcharkes avatar Dec 13 '23 10:12 dcharkes

Another solution would be to introduce a top-level directory called assets where build.dart and link.dart reside in.

HosseinYousefi avatar Dec 13 '23 10:12 HosseinYousefi

Having a directory would fit with a possibility of having pub resolve dependencies differently in a directory:

  • https://github.com/dart-lang/pub/issues/3794

Another solution would be to introduce a top-level directory called assets where build.dart and link.dart reside in.

Flutter already has an assets/ directory as well. Maybe it's a good thing to have Dart scripts in there, maybe it's a bad thing because the files in that directory are currently interpreted as non-executable and bundled as is? (https://docs.flutter.dev/ui/assets/assets-and-images)

dcharkes avatar Dec 13 '23 11:12 dcharkes

Having a directory would fit with a possibility of having pub resolve dependencies differently in a directory:

Yes! asset_dependencies.

Another solution would be to introduce a top-level directory called assets where build.dart and link.dart reside in.

Flutter already has an assets/ directory as well. Maybe it's a good thing to have Dart scripts in there, maybe it's a bad thing because the files in that directory are currently interpreted as non-executable and bundled as is? (https://docs.flutter.dev/ui/assets/assets-and-images)

The place for assets is not hardcoded in Flutter, but it's true that usually people use the name assets for the top-level directory containing their icons, fonts, ... . I don't think that they include any Dart scripts there, we could show a warning/error if they accidentally include dart files as assets (especially for build.dart and link.dart).

HosseinYousefi avatar Dec 13 '23 11:12 HosseinYousefi

I'd also prefer to not such scripts in the root directory.

Top level scripts are not part of the public surface of a package.

And with good reason, it's because they shouldn't be. That's prime real-estate that should belong to the author. Or for the author to ensure it's kept clean, if they don't want clutter. (I'd also move pubspec.lock into .dart_tool if we started creating it today.)

The tool directory already exists, otherwise it would make sense (or tools, but having both would be very confusing).

Probably not a .name, otherwise I'd go for .build. Hmm, maybe it can work. But not if some people are blanket-git-ignoring .*, which they might.

Maybe scripts? build_scripts? build_tools?

Do we expect more scripts to be added in the future, or is this going to be about FFI native assets forever. Maybe some scripts can be triggered by macros (say a macro which requires downloading and caching some specification, so it can depend on that while running).

If so, the script names should be less generic. Maybe asset_build.dart and asset_link.dart.

Is "asset" the best name, if that's already what people are using for their icons? (The name has always confused me when used about native resources, probably because I associate it strongly with web assets.)

Could it be ffi_build.dart, ffi_link.dart? That's specific enough to be precise. I might even understand what it is :)

lrhn avatar Dec 13 '23 15:12 lrhn

Could it be ffi_build.dart, ffi_link.dart? That's specific enough to be precise. I might even understand what it is :)

@mosuem wants to use the (to be built) link.dart to trim/tree-shake json files for internationalization/translation. So this is not FFI related.

Whatever comes out of build.dart and link.dart will end up in the final application bundle. And it's not compiled Dart code (most likely).

That being said, maybe we have a need for some kind of pre-defined script in the Dart/Flutter eco system that is not related to bundling assets (be it code or data), but is a predefined script name. So in that case even "asset" is not the right name either. Currently "asset" is the right name, but it might not be in the future.

Maybe scripts? build_scripts? build_tools?

I think I like the last one best, but it's long.

Python and Node use the term "hooks". Som we could reserve the folder hooks? hooks/build.dart. (But then "hooks" sound a lot like an implementation detail, like a "callback".)

dcharkes avatar Dec 13 '23 16:12 dcharkes

Will the scripts need to be dart files, or could we allow someone writing a build.sh or build.py instead? We pobably wouldn't allow publishing that, since it's platform dependent. So I guess they can write a build.dart which exec's python build.py.

lrhn avatar Dec 13 '23 17:12 lrhn

We require these scripts to be dart scripts. We can use the "current" Dart executable to run these scripts from where we want to run them. We could require the dart scripts to be marked executable and having dart on the path, but that means we cannot rely on that Dart executable being the same one as "the host". So I think that would be a bad idea.

So I guess they can write a build.dart which exec's python build.py.

Be my guest. 😅

dcharkes avatar Dec 13 '23 17:12 dcharkes

Do current build stack and web compilers migrate to this? I love to see something like zig build system. Typed. No yaml files.

ykmnkmi avatar Dec 14 '23 03:12 ykmnkmi

Do current build stack and web compilers migrate to this?

Web frameworks should not use a build.dart internally.

Dart command-line and Flutter apps consume the output of build.dart so that the assets are available on various target platforms. It would be conceivable that the web frameworks would also consume the assets that come out of build.dart. @mosuem briefly discussed this earlier.

I love to see something like zig build system. Typed. No yaml files.

If we standardize to a build system around Dart, it would be Bazel most likely. In which case build.dart scripts would most likely have to be rewritten in Bazel build rules. (Or we would provide some kind of Bazel build script that can invoke and consume the output of build.dart scripts.) cc @mraleph Some ammunition for your request.

dcharkes avatar Dec 14 '23 10:12 dcharkes

@devoncarew mentioned that npm has the concept of scripts defined in the package.json, their version of pubspec.yaml, see an example or the definition. These scripts are associated with "lifecycle events", and have the notion of being prefixed with Pre or Post, indicating when they should be run.

A similar concept could be applied here; having a top-level scripts key in the pubspec which defines all kinds of scripts. Use cases could be (thanks @dcharkes for collecting these)

  • Pre-Kernel-Compilation: Generate libraries with build.dart.
  • Post-Kernel-Compilation: Treeshake unused assets with link.dart.
  • Pre-Testing: Run test setups such as dart test/setup.dart in FFIgen.
  • Pre-Analyzing: Running dart pub get in a bunch of folders.
  • Pre-Pub-Publish: Run FFIgen in your repository to ensure you don’t upload any outdated generated files.

This could be defined in the pubspec as

scripts:
  pre-kernel-compilation:
    build
  post-kernel-compilation:
    link
  pre-testing: 
    test_setup

with a top-level scripts (or similar name, see https://github.com/dart-lang/sdk/issues/54334#issuecomment-1854139254) folder containing build.dart, link.dart, and test_setup.dart.

These features would help the ecosystem, even if they make the process slightly more magic. If a repository requires extra steps to be done before testing, running dart test test/my_test.dart would work without having to run some custom scripts manually first. Failures might however be harder to debug as the user doesn't just have to look at my_test.dart, but possibly also at test_setup.dart, which is not visible from looking only at my_test.dart.

mosuem avatar Dec 14 '23 15:12 mosuem

I love the idea of having a place to hook in to for making sure all generated code is indeed regenerated before dart pub publish publishes the files.

If we would go for "script", then the script/ directory as suggested by Lasse makes sense. (Together with a script_dependencies in the pubspec.yaml for that directory.)

A package layout would then be:

  • bin/ - public CLI of package - uses dependencies
  • lib/ - public Dart API of package - uses dependencies
  • script/ - lifecycle scripts that are run by Dart tooling (all scripts would have a pre-mandated CLI) - uses script_dependencies
  • tool/ - private scripts that can only be run from the package as root package - uses dev_dependencies
  • (root folder) the same as tool/, but really, don't.

dcharkes avatar Dec 14 '23 15:12 dcharkes

A few notes:

  • Package layout convention uses singular nouns. Hate it or love it, let's stay consistent.
  • Let's defer all discussion of tool_dependencies and script_dependencies to a separate orthogonal issue. These things won't block us now, but they are hard to get right and harder to undo.
  • I like the idea of hooks/scripts, proposed something similar in pub#4058. I think we should be very careful where we put them. NPM might not be an example to follow.

Honestly, I don't know that it would be unfair to put a script/ or hook/ folder inside either tool/ or bin/.

Probably we should be careful what hooks we introduce into the Dart SDK. But I see build.dart and link.dart making sense. Maybe, even publish.dart to build native artifacts (or generate code) before your package is published.

jonasfj avatar Dec 14 '23 17:12 jonasfj

@dcharkes there has been a lot of discussion around this breaking change, are we ready to push it up the approval queue?

itsjustkevin avatar Jan 08 '24 17:01 itsjustkevin

Based on the discussion in this issue and related issues, the two most probable options are:

  1. scripts/build.dart, script/link.dart, etc. and we'd reserve the whole script folder. All scripts would have a predefined CLI when introduced.
  2. toplevel build.dart and link.dart.

I'm not fully bought in to "script", but I believe it's the best option out of all the suggestions made in this issue and related issues.

If we want to go for option 1, then I'll rephrase the issue reserve the top level script folder. @mit-mit @mkustermann @mraleph @devoncarew @HosseinYousefi @mosuem @jonasfj @lrhn Any concerns? If not I think we should just chose it and move forward.

dcharkes avatar Jan 08 '24 18:01 dcharkes

At the risk of further bikeshedding on the name, perhaps reserving build and link script names in a workflow directory? So,

  • workflow/build.dart
  • workflow/link.dart

That wouldn't pollute the top-level directory w/ single scripts, and would be forwards compatible w/ any declarative workflow scripts defined in the pubspec file, if we choose to explore that (i.e., this comment).

devoncarew avatar Jan 08 '24 23:01 devoncarew

The more I think about it, the less I like script. It's too non-descript. It can mean anything.

If the directory is intended to hold multiple files, which may include data files, not just scripts, what is the concept that binds these files together, and which can be used as a filter when deciding whether a file good in there or not? "Script" fails to filter, any script that I wrote would fit that name.

If we're not filtering anyway, I'd just use tool, because that's where I'd put code generation scripts today. They're dev-tools, used while developing the code in lib and bin. If I have a script which generates Dart code, I'd put it in tool. But tool already exists, so it's hard to reserve, and risks conflict. But a subdirectory might work.

Devon suggests "workflow". Not sure it's precise enough, it doesn't say which workflow. I can think of many.

Same issue for lifecycle - of what?

Maybe build, if it's only used for things used to build a product?

So define, precisely, what the directory is intended to be used for. Find a single concept which clearly and precisely covers that (what is it, what is it not). Use that concept as name. (Easy-peasy, just like any other naming! 😅)

Then consider whether it can be a subdirectory of tool instead of a top-level directory.

lrhn avatar Jan 09 '24 08:01 lrhn

Then consider whether it can be a subdirectory of tool instead of a top-level directory.

One issue with making it a subdirectory is the discoverability. If we're going to run package author code automatically on users machines, (on pub get, on build, on ...) I think that that should be easy to spot.

  • The toplevel build.rs has that property. - But it only does building, so it's easy to give it the name of the specific lifecycle event it's happening in 'build'.
  • PyInstaller specifies things in a config file - so, no predefined folder or file name. The config entry is called 'hook' (and it specifies a directory with hooks).
  • NPM specifies things in a config file - so, no predefined folder or file name. The config entry is called 'script'.

So, Rust goes for a single concept, and it only has one file. NPM and Python go for multiple concepts and give the thing a generic name such as 'hook' or 'script'.

Because we've got more use cases than just build.dart and link.dart, also https://github.com/dart-lang/sdk/issues/54334#issuecomment-1856051593 and https://github.com/dart-lang/pub/issues/4058. I'm leaning towards selecting a generic top-level folder that we then give a special meaning. Hence the toplevel script/. Toplevel means it's visible. Generic it means we can put different types of scripts in there. The 'script' name is generic, but a top level directory is easy to document on https://dart.dev/tools/pub/package-layout. It needs to be a singular noun. And we reserve it, define its meaning, and document it.

dcharkes avatar Jan 09 '24 09:01 dcharkes

I worry that the name script is too good. I'd be tempted to put my own scripts in there too. You can say that we reserve the entire directory, but if we don't enforce it, we risk some people stuffing their own scripts in there. Perfectly reasonable scripts even, might be called by their build systems.

So consider something less attractive, like tool_hook. That's a name that nobody will get envious about, so we might keep it to ourselves.

(We should still show warnings of someone adds an unrecognized script in there, and day that if the recognized scripts need helper scripts, put those under tool_hook/src.)

lrhn avatar Jan 10 '24 22:01 lrhn

I think hooks/ might be a good option.

mit-mit avatar Jan 11 '24 09:01 mit-mit

I think hooks/ might be a good option.

Or rather hook/ to be consistent with the singular naming scheme.

HosseinYousefi avatar Jan 11 '24 10:01 HosseinYousefi

~~Be aware that Git, by default, uses hooks/ as a folder for having git related hooks: https://git-scm.com/docs/githooks/ .~~

~~So for some projects, it is possible that there are already a hooks/ folder in the project. Could then be a bit confusing that the project also have a hook/ folder which are then used by Dart.~~

My bad. That folder are inside the .git folder.

julemand101 avatar Jan 11 '24 10:01 julemand101

Be aware that Git, by default, uses hooks/ as a folder for having git related hooks: https://git-scm.com/docs/githooks/ .

So for some projects, it is possible that there are already a hooks/ folder in the project. Could then be a bit confusing that the project also have a hook/ folder which are then used by Dart.

I usually see the Git hooks directory to be .git/hooks/ which doesn't seem to be that confusing to me.

HosseinYousefi avatar Jan 11 '24 10:01 HosseinYousefi

@HosseinYousefi Yeah you are right. I got quite confused there :)

julemand101 avatar Jan 11 '24 10:01 julemand101

So, are we going for hook/?

dcharkes avatar Jan 23 '24 21:01 dcharkes

🪝 SGTM

mit-mit avatar Jan 24 '24 08:01 mit-mit

I have updated the issue description to say that we are reserving the hook/ toplevel directory.

@itsjustkevin This is ready to go through the breaking change approval queue.

dcharkes avatar Jan 24 '24 11:01 dcharkes

Thanks @dcharkes!

@vsmenon @Hixie @grouma breaking change request for review.

itsjustkevin avatar Jan 24 '24 13:01 itsjustkevin

fyi @christopherfujino @jonahwilliams @dnfield @jmagman @reidbaker

As far as reserving a top-level directory goes, I don't object. What is the user experience like when they have a conflict today?

Hixie avatar Jan 24 '24 19:01 Hixie

Can we please pick a different name that is less likely to conflict?

Is there some reason this can't go under .dart_tool? Could we call it .dart_hooks or some such? It would very likely not break anyone if we picked such a scheme.

dnfield avatar Jan 24 '24 20:01 dnfield