-
Notifications
You must be signed in to change notification settings - Fork 42
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
Keep track of adjusted and original dotfile paths #63
Conversation
I added another commit making the same change for unstowing too. I might add some more tests if I have the time, but I think this should be working now. I'm not exactly sure why the CI build failed, it looks like it's failing to get the right perl version and thus can't access the module needed to run the tests. Afaict it's not related to this change. |
I'm not too familiar with Travis CI, but it seems that the default build environment (Ubuntu 16.04) does not have the Perl archives listed in Ubuntu 14.04 does provide those Perl versions though, so I think adding |
I'll probably have some time early in the NY to catch up on Stow stuff. Way too busy right now, sorry. |
Is there any plans of merging this PR? |
I definitely hope so! Or if not this, some other solution which satisfies as many people as possible. Unfortunately due to other commitments I'm hoping to catch up on the Stow backlog in a single batch - some time in the next few weeks is probably realistic. Really sorry for the delay. |
This also fixes #77, I added some extra tests to confirm: Barabas5532@d4dceac. Any news on when this can be merged? |
Unfortunately it can't be merged as is because it breaks the tests. But I'm very open to the possibility of merging a fix for this issue once it's clear what the best solution is. I'm having problems understanding the approach taken here though. @pvsr Is there any chance you could explain the idea behind this change? Ideally the commit messages would fully explain any changes in logic, and why they're needed. Thanks! |
I believe the bug that I was trying to fix was that the sections of code I modified were using dotfile-adjusted versions of file names (i.e. ".config") to reference files within the source package, where they live under the non-adjusted name (i.e. "dot-config"). I was trying to fix that by keeping track of unadjusted names (package_target) and converting them to adjusted versions as needed. But that was a hacky solution that broke on edge cases I didn't understand. Unfortunately I can't tell you much more than that because I wrote this a long time ago and I find the code hard to follow. I'm not interested in getting this merged because I no longer use stow, but anyone who wants to is free to use this PR as a base for trying to fix dotfiles. |
Thanks @pvsr. I don't personally use |
The tests are passing for me, am I doing something wrong?
Are you talking about the failed CI build in travis? That seems to be because of missing modules. |
@Barabas5532 commented on February 15, 2021 6:53 PM:
Were you running them on this PR branch?
Yes.
I don't think so because a) |
Could it |
Ah thanks, I missed that. Yeah could be. Although I thought I saw other errors when (hastily) scanning the build log. |
You can still access the travis logs by clicking the red x next to commits above. Here is a link to the latest one. It fails here, but keeps going for some reason:
Also later: |
#70 has been merged into the See here for more details: #70 (comment) |
This is obviously a lot more invasive than the original approach, so I'm not sure if it's acceptable, but it's probably necessary to fix #33. I'm not really happy about having to repeatedly adjust the target in each new call to stow_contents and stow_node. Perhaps it would be better to pass
$package_target
and$target
as separate arguments?