buildinspace / peru

a generic package manager, for including other people's code in your projects
MIT License
1.11k stars 69 forks source link

overriding a recursive module fails if the override is synced #125

Open oconnor663 opened 9 years ago

oconnor663 commented 9 years ago

When you have a module with a peru.yaml of its own, its imports get fetched and merged as part of its own tree. That applies even if the module is overridden (assuming the override path also contains peru.yaml). However, if peru sync has been run at the override path, the imports will already be present in the override tree. Thus when peru tries to merge them in itself, there will be a conflict. It's not immediately clear how we should solve this.

The simplest thing would be to make the rule that "override trees don't get recursive imports applied to them." The problem with that is that now using a fresh clone as an override wouldn't work, because the recursive imports would be missing. You'd have to remember to do a sync yourself. (This is reminiscent of when we used to run build commands inside modules -- the interaction of this with overrides was complicated.)

We could try a more complicated solution. For example, we could attempt to merge imports, but allow a merge failure and just print a warning. It's possible that that could end up being exceptionally confusing in some cases (like your imports were there until you touch'ed some random file, and now they're totally gone, and you didn't notice the warning in your server's logs...), but it might do what people want 99% of the time.

Or there are other options, where maybe we allow different override modes or top level flags. At the expense of more and more complexity...

Running into this also highlighted two different but related issues:

oconnor663 commented 9 years ago

Note that this is kind of related to #76 too. Both are cases of files being in the override dir that aren't normally in the module.

oconnor663 commented 9 years ago

Another note: in https://github.com/buildinspace/peru/commit/00580eb18c6eee6aee2f0267199fbde8a55512e9 we explicitly ignored the .peru dir in overrides, because we used to run a sync right there in the override dir. We might want to remove that logic now that we try not to modify overrides ourselves.

oconnor663 commented 8 years ago

@olson-sean-k and I talked about this today. The right way to solve tricky override interactions with .hg/recursion/whatever will probably be to add more metadata to the override feature, so that the user can manually exclude things. We have the option of waiting for a real use case to show up and doing this after 1.0.

That said, the commit above about the .peru directory is important. We might actually want more special casing there, not just for overrides. I'll open a new issue.

MatthewEppelsheimer commented 7 years ago

+1 to addressing this. I've run into this and learned to manually peru clean recursive overrides before syncing.

oconnor663 commented 7 years ago

Here's a different idea: What if we ran a real sync inside your override before we slurped up all its files, instead of running the sync "on the side" and merging it in after the fact? That would modify your override dir, which could be a little bit unexpected, but then again it's exactly what we used to do back when we had a build field. I think it would solve both the "I just cloned this repo" case and the "I am actually doing work in this repo" case at the same time. (And presumably if you have a peru.yaml file at the root of your repo, and your callers are syncing it recursively, it's unlikely to break anything if we sync it in place?) What do you think, @olson-sean-k?

MatthewEppelsheimer commented 7 years ago

This is a tricky one. As a user, I'm not quite sure how to resolve this ideally.

Running a real sync inside of the override still requires the user to pay close attention to how they're using the override directory, in much the same way as doing a peru clean in an override before syncing an external project. With the added downside of modifying the override directory in potentially unexpected ways, that's not not a slam dunk.

Another idea: Just ignore the recursive field for overrides. This still requires the user to be intentional about the override's state, but at least preserves the WIP benefits of overrides. This way the user can decide whether to apply a clean sync for the override, or to make modifications to it instead — whereas forcing a sync of the override removes that option, and potentially interferes with whatever work the user was doing in an unclean override.

A downside is this could be annoying and/or confusing for users who are unaware of the behavior. So if you go this route, it may be worth outputting a notice, e.g. Skipping recursive syncing for overrides.

oconnor663 commented 7 years ago

Running a real sync inside of the override still requires the user to pay close attention to how they're using the override directory

Can you give me some examples of things this would break, to help my thinking?