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.

Sign up for GitHub

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

Jump to bottom

clone from RIA store (kinda) fails with invalid TMPDIR #6514

Closed
bpoldrack opened this issue Mar 1, 2022 · 3 comments · Fixed by #7147
Closed

clone from RIA store (kinda) fails with invalid TMPDIR #6514

bpoldrack opened this issue Mar 1, 2022 · 3 comments · Fixed by #7147
Labels
RIA/ORA Issues related to RIA-based workflows and related components team-core core API/commands (https://github.com/datalad/datalad/issues/6365) team-git Git interface (GitRepo, protocols, helpers, ...) (https://github.com/datalad/datalad/issues/6365) UX user experience

Comments

@bpoldrack
Copy link
Member

A user reported an issue where she wouldn't be able to get a subdataset resulting in:

[INFO   ] Fetching 'http://store.datalad.org/d3f/a72e4-2c2b-11ea-948f-0025904abcb0/config' 
[ERROR  ] FileNotFoundError([Errno 2] No such file or directory: '/tmpstore/datalad_temp_9lpj5jqp') (FileNotFoundError)

Turns out she had an invalid TMPDIR=/tmpstore. The call succeeded on second try, though and it looked like it would behave differently when called with DEBUG logging.

Culprit is: clone is putting the content of the store's config into a tempfile in order to let a call to git config parse it. This is happening post-clone, so the actual cloning has already succeeded, explaining the observed differences in behavior. However, this partial success is not recognizeable to the user.
Moreover, the need for the tempfile is questionable to begin with, as this comment in the code states:

# TODO: We might be able to spare the saving to a file.
#       "git config -f -" is not explicitly documented but happens
#       to work and would read from stdin. Make sure we know this
#       works for required git versions and on all platforms.

So, two things need to be done, from my POV:

  1. Make sure we don't crash in a postclone routine, obfuscating the fact that we already succeeded with the cloning (or crash but then clean up the clone)

  2. As the comment says: If we can confirm this to work reliably, we should switch and not bother with a tempfile at all.
@bpoldrack bpoldrack added team-core core API/commands (https://github.com/datalad/datalad/issues/6365) team-git Git interface (GitRepo, protocols, helpers, ...) (https://github.com/datalad/datalad/issues/6365) labels Mar 1, 2022
@adswa adswa added the UX user experience label Mar 1, 2022
@yarikoptic
Copy link
Member

I would not rely on a system/setup which "seemingly works" with an incorrect setting of TMPDIR. I would not mind avoiding tempfiles though, but as far as original issue, IMHO it is a user-error.

@bpoldrack
Copy link
Member Author

It is a user-error, but it's unclear to the user what happened (we actually installed the subdataset despite the command failing) and what didn't (setting up a potential ORA remote properly).

@mih mih added the RIA/ORA Issues related to RIA-based workflows and related components label Mar 25, 2022
@adswa adswa mentioned this issue Nov 7, 2022
Closed
adswa added a commit to adswa/datalad that referenced this issue Nov 7, 2022
@adswa
BF: read config from stdin instead of temporary file
40c337b 
as outlined in a comment by @bpoldrack in the code, git config -f - can
read from stdin. This is supported by git as its a unix convention.
Trial and error confirms that this also runs successfully in a Windows
CMD. By switching to a git config call with stdin as the input file, we
spare the need to write a temporary config file.
This fix could thus fix datalad#6514, where a user error lead to a non-existent
TMP path and a subsequent failure to create the temporary config file.
@adswa adswa mentioned this issue Nov 7, 2022
Merged
adswa added a commit to adswa/datalad that referenced this issue Nov 17, 2022
@adswa
BF: read config from stdin instead of temporary file
ff16b49 
as outlined in a comment by @bpoldrack in the code, git config -f - can
read from stdin. This is supported by git as its a unix convention.
Trial and error confirms that this also runs successfully in a Windows
CMD. By switching to a git config call with stdin as the input file, we
spare the need to write a temporary config file.
This fix could thus fix datalad#6514, where a user error lead to a non-existent
TMP path and a subsequent failure to create the temporary config file.
@yarikoptic-gitmate
Copy link
Collaborator

Issue fixed in 0.17.10

@mih mih mentioned this issue Mar 1, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
RIA/ORA Issues related to RIA-based workflows and related components team-core core API/commands (https://github.com/datalad/datalad/issues/6365) team-git Git interface (GitRepo, protocols, helpers, ...) (https://github.com/datalad/datalad/issues/6365) UX user experience
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

BF: read RIA config from stdin instead of temporary file adswa/datalad
5 participants
@yarikoptic @mih @bpoldrack @adswa @yarikoptic-gitmate