-
Notifications
You must be signed in to change notification settings - Fork 521
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 escript_incl_priv so escriptize priv dir inclusion works with _checkouts and profiles #2602
Add escript_incl_priv so escriptize priv dir inclusion works with _checkouts and profiles #2602
Conversation
{escript_incl_extra, [ | ||
{"relx/priv/templates/*", "_build/prod/lib/"}, | ||
{"rebar/priv/templates/*", "_build/prod/lib/"} | ||
]}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a bonus, these overrides for the prod
profile can be dropped since the new implementation computes the output directory (using the current profiles in effect).
903c0e1
to
daf3c8e
Compare
rebar.config
Outdated
{escript_incl_extra, [{"relx/priv/templates/*", "_build/default/lib/"}, | ||
{"rebar/priv/templates/*", "_build/default/lib/"}]}. | ||
{escript_incl_extra, [{relx, "priv/templates/*"}, | ||
{rebar, "priv/templates/*"}]}. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this works as a change. This is moving from a wildcard within any directory into something that becomes specific to OTP applications. I.e. an escript might want to bundle things unrelated to OTP apps, and there is no way to maintain that behaviour with the new configuration method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point @ferd. I can generalize the implementation of escript_incl_extra
to not limit its use to OTP apps. To help steer this work, could you please clarify the following?
escript_incl_extra
is currently limited torebar3
-only internal use, but will it likely become available for other projects?- What use cases are intended to be supported by
escript_incl_extra
? For instance:- A directory generated by
rebar3
(e.g.,_build/<profile>/lib/
) - A directory under the project root (
rebar.config
), but not generated byrebar3
(e.g.,priv/
) - A directory outside of the project root (e.g.,
/some/other/dir/
). If any of these use cases are to be ruled out, I'd guess it's this one.
- A directory generated by
To support (2) and (3), it seems sufficient to allow static paths (as escript_incl_extra
accepts today).
To support (1), are there additional use cases beyond an output directory of an OTP app?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We intended for it to only be used by rebar3, but as with all things, people who needed to bundle priv dirs in escripts started relying on it without us ever having improved the mechanism. So while its usage is discouraged, we're fully aware that changing behaviour will break random apps for people.
The use cases we have it for is just carrying priv/ dirs into apps, but not necessarily all of them. So we just did the quick workaround by adding a filepath with a regex. To carry the relx files, it had to be in _build since we don't have it from the root right away, but someone could use any path whatsoever, whether absolute or relative. It's messy, which is the reason we never really made the feature official.
{escript_priv_dir, [app | {app, Filter}]}
would possibly be a nicer way to support this, that would partially overlap with the existing feature, but I don't know if it'd be cleaner.
A whole other approach would be the rebar.config.script trick doing detection of _checkouts and changing the paths accordingly, but that's extremely hackish.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just implemented. Do you think it is better to limit the new include wildcards to the app priv/
directory (as it is with this PR right now), or to keep it general, despite the rebar3
use case not needing it?
Also, the PR deviates somewhat from the app | {app, Filter}
suggestion by only supporting {app, Filter}
. The rationale here is to limit the different syntaxes which would need to be supported. Plus, {app, "*"}
is arguably not that much worse to type out if needed. 😉 Of course, if there is a strong preference to retain it, it wouldn't be difficult to do so.
When developing `relx` and testing changes with `rebar3`, it is convenient to make `relx` a checkout dependency. However, the current implementation of `escript_incl_extra` expects the output directory of `relx` to be under `_build/<profile>/lib/`, which is not the case if `relx` is a checkout dependency. Instead of making a breaking change to `escript_incl_extra`, introduce a new config parameter instead.
daf3c8e
to
6d78d19
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, this looks solid! Thanks for the contribution there again. I believe the failing CI test is unrelated and is due to a third party plugin having a bad interaction since this feature logically has no overlap with it. This is going in.
When developing
relx
and testing changes withrebar3
, it is convenient to makerelx
a checkout dependency.However, the current implementation of
escript_incl_extra
expects the output directory ofrelx
to be under_build/<profile>/lib/
, which is not the case ifrelx
is a checkout dependency.Fixes #2600
Note
Due to the change in
rebar.config
, this is a backwards-incompatible change when using an older version ofrebar3
to build a new version ofrebar3
. But because the recommended way to buildrebar3
is with./bootstrap
, perhaps this is okay.Test plan
rebar3
relx
atrebar3/_checkouts/relx
rebar3
using./bootstrap
${path-to-rebar3-repo}/rebar3 release
on another projectrebar3
crashes as reported inrebar3 release
crashes when bootstrapped withrelx
as a checkout dependency #2600./escript-archive-list ${path-to-rebar3-repo}/rebar3
(see below)relx/priv
paths do not appear in therebar3
escript
archiveescript