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

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

Closed
3 tasks done
dcharkes opened this issue Dec 13, 2023 · 48 comments
Closed
3 tasks done

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

dcharkes opened this issue Dec 13, 2023 · 48 comments
Labels
area-sdk Use area-sdk for general purpose SDK issues (packaging, distribution, …). breaking-change-approved breaking-change-request This tracks requests for feedback on breaking changes enhancement-breaking-change An enhancement which is breaking.

Comments

@dcharkes
Copy link
Contributor

dcharkes commented Dec 13, 2023

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:

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.

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

Impact

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

We are aware of one use:

Mitigation

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

Action items after accepting proposal:

@dcharkes
Copy link
Contributor Author

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

@HosseinYousefi
Copy link
Member

HosseinYousefi commented Dec 13, 2023

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

@dcharkes
Copy link
Contributor Author

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

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)

@HosseinYousefi
Copy link
Member

HosseinYousefi commented Dec 13, 2023

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).

@lrhn
Copy link
Member

lrhn commented Dec 13, 2023

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 :)

@dcharkes
Copy link
Contributor Author

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".)

@lrhn
Copy link
Member

lrhn commented Dec 13, 2023

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.

@dcharkes
Copy link
Contributor Author

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. 😅

@kevmoo kevmoo added the area-sdk Use area-sdk for general purpose SDK issues (packaging, distribution, …). label Dec 14, 2023
@ykmnkmi
Copy link
Contributor

ykmnkmi commented Dec 14, 2023

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

@dcharkes
Copy link
Contributor Author

dcharkes commented Dec 14, 2023

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.

@mosuem
Copy link
Member

mosuem commented Dec 14, 2023

@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 #54334 (comment)) 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.

@dcharkes
Copy link
Contributor Author

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.

@jonasfj
Copy link
Member

jonasfj commented Dec 14, 2023

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.

@itsjustkevin
Copy link
Contributor

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

@dcharkes
Copy link
Contributor Author

dcharkes commented Jan 8, 2024

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.

@devoncarew
Copy link
Member

devoncarew commented Jan 8, 2024

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).

@lrhn
Copy link
Member

lrhn commented Jan 9, 2024

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.

@dcharkes
Copy link
Contributor Author

dcharkes commented Jan 9, 2024

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 #54334 (comment) and dart-lang/pub#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.

@lrhn
Copy link
Member

lrhn commented Jan 10, 2024

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.)

@mit-mit
Copy link
Member

mit-mit commented Jan 11, 2024

I think hooks/ might be a good option.

@HosseinYousefi
Copy link
Member

I think hooks/ might be a good option.

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

@julemand101
Copy link
Contributor

julemand101 commented Jan 11, 2024

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.

@HosseinYousefi
Copy link
Member

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.

@julemand101
Copy link
Contributor

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

@dnfield
Copy link
Contributor

dnfield commented Jan 24, 2024

  1. Why can't these files/locations were encoded in the pubspec.yaml?
  2. How will I, as a package consumer, enable or disable these scripts from running?
  3. What will the failure mode be if the script is malformed or incompatible with the Dart SDK I have installed?
  4. Will the script have full access to dart:io and whatever priviledges the current user has?

@dcharkes
Copy link
Contributor Author

  1. Why can't these files/locations were encoded in the pubspec.yaml?

We did consider this (#54111) (for example nodejs does this with a scripts key in their config file), but it causes eco-system fragmentation. We'd like to follow Rust's model of a predefined path.

  1. How will I, as a package consumer, enable or disable these scripts from running?

You can't and you shouldn't. The package you're consuming does not work without executing these scripts. See #50565 and flutter/flutter#129757.

  1. What will the failure mode be if the script is malformed or incompatible with the Dart SDK I have installed?

An error message when trying to do flutter run, dart build, ... see the test cases added in the various PRs implementing this experimental feature.

  1. Will the script have full access to dart:io and whatever privileges the current user has?

Yes. You're going to build C libraries (process invocation to a C compiler). Or download files from a CDN.

@dnfield
Copy link
Contributor

dnfield commented Jan 24, 2024

You can't and you shouldn't. The package you're consuming does not work without executing these scripts. See #50565 and flutter/flutter#129757.

This seems like it will cause problems for platforms/build environments the author didn't consider properly. For example, a significant number of projects we support build using a Bazel -like build system. We have users who want to build and run applications in environments with musl instead of glibc. We have users who build using MSVC, gcc, clang, apple-clang. We have a number of architectures to support.


With regard to the original suggestion, I'm not "opposed enough" to hooks/ to say either way. I think @christopherfujino should definitely get some input here though.

@itsjustkevin
Copy link
Contributor

Checking in again on this breaking change request @dcharkes

@dcharkes
Copy link
Contributor Author

dcharkes commented Feb 5, 2024

With regard to the original suggestion, I'm not "opposed enough" to hooks/ to say either way.

It should be hook/ not hooks/ #54334 (comment).

I think @christopherfujino should definitely get some input here though.

cc @christopherfujino Any concerns from your side?

@reidbaker
Copy link
Contributor

You can't and you shouldn't. The package you're consuming does not work without executing these scripts. See #50565 and flutter/flutter#129757.

This seems like it will cause problems for platforms/build environments the author didn't consider properly. For example, a significant number of projects we support build using a Bazel -like build system. We have users who want to build and run applications in environments with musl instead of glibc. We have users who build using MSVC, gcc, clang, apple-clang. We have a number of architectures to support.

With regard to the original suggestion, I'm not "opposed enough" to hooks/ to say either way. I think @christopherfujino should definitely get some input here though.

@dcharkes what is your recommendation for people using a another build system (like bazel)?

@mraleph
Copy link
Member

mraleph commented Feb 5, 2024

@dcharkes what is your recommendation for people using a another build system (like bazel)?

People using custom build systems will naturally have to do custom stuff, there is no silver bullet. Similar to how Pub packages don't automatically come with BUILD files for Dart code - requiring one to author build rules, they will not magically come with BUILD files for native code. There are different ways we could do this integration (for simple cases we could even autogenerate necessary BUILD files), but nothing is included into the initial version which we are planning to ship. We should certainly have a good amount of documentation to explain how this integration should be done and how native assets work underneath.

That being said this discussion certainly does not belong to this particular breaking change.

If there are some specific concerns @reidbaker @dnfield - I encourage you to suggest to fork a separate issue with details or start a mail thread.

@christopherfujino
Copy link
Member

With regard to the original suggestion, I'm not "opposed enough" to hooks/ to say either way.

It should be hook/ not hooks/ #54334 (comment).

I think @christopherfujino should definitely get some input here though.

cc @christopherfujino Any concerns from your side?

SGTM

@itsjustkevin
Copy link
Contributor

Thanks @dcharkes and squad, sending this back into the breaking change approval ether!

@Hixie @vsmenon @grouma could you take a look at this breaking change request?

@grouma
Copy link
Member

grouma commented Feb 5, 2024

I did a quick search and this shouldn't impact internal code. SGTM.

@Hixie
Copy link
Contributor

Hixie commented Feb 6, 2024

An error message when trying to do flutter run, dart build, ... see the test cases added in the various PRs implementing this experimental feature.

Does the error message explain the magical nature of these files? That would mitigate the breakiness significantly.

Anyway, I still have no objection.

@vsmenon
Copy link
Member

vsmenon commented Feb 7, 2024

lgtm

@dcharkes
Copy link
Contributor Author

dcharkes commented Feb 15, 2024

Thanks everyone for your input! 🙏

I'll start working on:

And close this issue once it's done.

dcharkes added a commit to dart-lang/site-www that referenced this issue Feb 21, 2024
parlough added a commit to dart-lang/site-www that referenced this issue Feb 26, 2024
Specify the `hook/` directory in the package layout.

* dart-lang/sdk#54334

---------

Co-authored-by: Jonas Finnemann Jensen <[email protected]>
Co-authored-by: Parker Lougheed <[email protected]>
atsansone pushed a commit to atsansone/site-www that referenced this issue Mar 22, 2024
Specify the `hook/` directory in the package layout.

* dart-lang/sdk#54334

---------

Co-authored-by: Jonas Finnemann Jensen <[email protected]>
Co-authored-by: Parker Lougheed <[email protected]>
@dcharkes
Copy link
Contributor Author

This change is now part of Dart and Flutter on the main branch.

@github-project-automation github-project-automation bot moved this from In review to Complete in Breaking Changes Mar 25, 2024
copybara-service bot pushed a commit that referenced this issue Sep 3, 2024
`hook/` is a reserved directory.
#54334

And the two hooks currently in existence require to be invoked with
the same dependencies as the root package. Hence the dependencies of
the hooks should be normal dependencies and not dev dependencies.

Bug: dart-lang/native#160
Change-Id: I7cf48659bd240c2e46b5bdede2a336763301b8c5
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/383301
Reviewed-by: Samuel Rawlins <[email protected]>
Commit-Queue: Daco Harkes <[email protected]>
Reviewed-by: Hossein Yousefi <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-sdk Use area-sdk for general purpose SDK issues (packaging, distribution, …). breaking-change-approved breaking-change-request This tracks requests for feedback on breaking changes enhancement-breaking-change An enhancement which is breaking.
Projects
Status: Complete
Development

No branches or pull requests