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

Do not apply overrides to a root application. #2083

Merged
merged 1 commit into from
May 21, 2019

Conversation

ferd
Copy link
Collaborator

@ferd ferd commented May 19, 2019

Overrides should apply to a layer below where they are declared. This
patch makes it so if the project root is an application (i.e. it isn't
'root' and therefore not an umbrella project), we omit applying
overrides in rebar_app_discover.

This in turn required changing a bunch of tests, because all the tests
worked with the idea that all overrides applied to all apps to validate
that they get inherited properly. The changes re-structure the cases so
they are written with an umbrella app, demonstrating that the changes
stick.

This addresses #2082 partially (still need to add docs)

The big risk here is whether any important project in the wild
relied on the bad application of at-large overrides to make their main
app in a non-umbrella work. I'm thinking the chance is low enough (it
would have been accidental and not "ah yes this is how the feature ought
to work", IMO) that this PR might be worth it.

Overrides should apply to a layer below where they are declared. This
patch makes it so if the project root is an application (i.e. it isn't
'root' and therefore not an umbrella project), we omit applying
overrides in rebar_app_discover.

This in turn required changing a bunch of tests, because all the tests
worked with the idea that all overrides applied to all apps to validate
that they get inherited properly. The changes re-structure the cases so
they are written with an umbrella app, demonstrating that the changes
stick.
@ferd ferd requested a review from tsloughter May 19, 2019 14:58
Copy link
Collaborator

@tsloughter tsloughter left a comment

Choose a reason for hiding this comment

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

I'm a little worried about it but it makes sense to do.

@ferd
Copy link
Collaborator Author

ferd commented May 19, 2019

At the very least it should have no effect on any dep I think, since those overrides would never have been applied to themselves in the first place (they don't go through app discovery on the first build if they haven't been fetched?).

At worst, we'll break the top-level project build only, but the deps would already have been broken. @tsloughter I think this de-risks it a lot since at least we don't risk breaking deps of existing projects, just the roots. That significantly reduces the network effect.

@ferd ferd merged commit 4b73301 into erlang:master May 21, 2019
@ferd ferd deleted the dont-apply-overrides-to-root branch May 21, 2019 14:33
jfacorro added a commit to clojerl/clojerl that referenced this pull request Jun 2, 2019
@jfacorro jfacorro mentioned this pull request Jun 2, 2019
jfacorro added a commit to clojerl/clojerl that referenced this pull request Jun 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants