Skip to content

Expand tilde using the current home path #954

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

Closed
wants to merge 1 commit into from

Conversation

afbjorklund
Copy link
Member

For instance it might be C:\Users\anders on the host,
but translated to /c/Users/anders path in the guest.

For instance it might be C:\Users\anders on the host,
but translated to /c/Users/anders path in the guest.

Signed-off-by: Anders F Björklund <anders.f.bjorklund@gmail.com>
@jandubois
Copy link
Member

jandubois commented Jul 18, 2022

I have a hard time making up my mind about this PR:

At some level I feel that u.HomeDir as returned by osutil.LimaUser should be set to /home/$USER.linux and not to the mount point of the host home directory. I know that we never did this before either. It was automatically set to the home directory of the host via user.Current(), but this was not done intentionally, and we did not use the field anywhere, so we never noticed.

So for convenience is seems like a good place to stuff the mountpoint, but I feel it would be cleaner to set the field to the actual home directory (value of $HOME inside the VM), and provide a separate function to return the mount point of the host home directory. I think it would also make the code more readable.

Now, I'm also not sure if localpathutil is the right package name anymore for an ExpandHome function that is essentially just a string replacement function now. It is confusing because it isn't clear if local refers to the host or the guest filesystem, but so far I assumed it meant "host".

Finally, there is one more spot that I think may need to use the adjusted HomeDir value for tilde expansion:

mountPoint, err := localpathutil.Expand(m.MountPoint)

@afbjorklund
Copy link
Member Author

This would need some revisiting, if we want the default mounts to work:

mounts:
- location: "~"
- location: "/tmp/lima"
  writable: true

This needs to be expanded to the (converted) host path, not the guest path.

Should /home/anders be mounted under /home/anders.linux, it breaks...


The refactoring points are valid, it is not really "localpath" util anymore.

And it seems like a fair point, to not override the real HomeDir of the user.

@jandubois
Copy link
Member

jandubois commented Jul 19, 2022

This would need some revisiting, if we want the default mounts to work:

I'm not asking for the mounts to change. I was just recommending to create a separate function to return the mount point inside the VM of the home directory on the host. This is a different thing than the user's home directory inside the VM and has no relation with what osutil.LimaUser() should return as the HomeDir.

I'm arguing that it was a bug that HomeDir was set to the path of the home directory on the host before, and you just extended it to the Windows case as well. I'm arguing that we should fix the bug and then use u.HomeDir in all the places where we currently do fmt.Sprintf("/home/%s.linux", u.Username) or similar.

And then create a separate function to return the LIMA_CIDATA_HOSTHOME_MOUNTPOINT and use that in all the places that need that value.

@jandubois
Copy link
Member

Note that the user can specify the mountpoint now, so the simplistic translation is not necessarily correct:

mounts:
- location: "~"
  mountPoint: /var/hosthome

So the function will need to check the definitions from lima.yaml as well.

@afbjorklund
Copy link
Member Author

afbjorklund commented Jul 20, 2022

Sounds good to return the meaning of home dir (to match that of the user), and adjust the code.

Originally I just though it was path (/) vs. filepath (\), but it turned out to be slighly more involved...

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