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

Flipped default of incompatible_relative_workspace_path to true #204

Merged
merged 4 commits into from
Oct 15, 2020
Merged

Flipped default of incompatible_relative_workspace_path to true #204

merged 4 commits into from
Oct 15, 2020

Conversation

UebelAndre
Copy link
Collaborator

This is a followup of #185 where an incompatibility flag was added that gates the new behavior introduced there in an effort to avoid a breaking change for current users.

However, I feel given enough time, the incompatibility flag can be removed and the forked behavior can be cleaned up. This is just a change to the default so users may have the more intuitive behavior and users still reliant on the previous behavior can flip the incompatible_relative_workspace_path setting to restore it.

This change will likely require an update to the root README.md. I would love some thoughts on how this could best be rolled out and what specifically people think should be updated in the README.md file.

@UebelAndre
Copy link
Collaborator Author

Now that there's be some kind of release (see #216) I think a change like this could be merged. It doesn't have to be immediately (especially given that there doesn't seem to have been any release notes informing people of the change) but I do think this could be merged and a notice posted somewhere to give users a chance to prep for the migration.

@acmcarther
Copy link
Member

What does this mean for users that have their Cargo.toml in /cargo? Will they still work?

@UebelAndre
Copy link
Collaborator Author

This means workspace_path will actually determine where raze outputs it's contents.

.
├── Cargo.toml
└── lib
    └── cargo

Given the structure above, you can currently run raze with worskpace_path = "//lib/cargo" and raze will always dump it's outputs next to the Cargo.toml file. This is incorrect since the paths in the generated outputs will expect this to be in //lib/cargo when it's not.

Setting incompatible_relative_workspace_path to true will cause raze to output it's contents to match workspace_path.

@acmcarther
Copy link
Member

I don't really understand the change that motivated the original flag. My concern is "will changing this default break existing users". Or, "do users using the old model need to be made aware of this change-in-defaults" so that they can do something.

@UebelAndre
Copy link
Collaborator Author

I don't really understand the change that motivated the original flag. My concern is "will changing this default break existing users". Or, "do users using the old model need to be made aware of this change-in-defaults" so that they can do something.

To the best of my knowledge, feature this flag toggles would not have broken existing behavior. The only case where it might have would be if users ran raze using --output. But that path would still ultimately have to align with the value of workspace_path. This flag should simply be removed but toggling it would still give people a chance to complain and go back to some behavior I can't think of.

@UebelAndre
Copy link
Collaborator Author

So to be clear, no I don't believe this change will break existing users.

@acmcarther
Copy link
Member

Alright, sgtm.

@acmcarther acmcarther merged commit 453e3e1 into google:master Oct 15, 2020
@UebelAndre UebelAndre deleted the remove-regression-test branch October 15, 2020 17:27
@UebelAndre
Copy link
Collaborator Author

@acmcarther Can you release a patch release for this?

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