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

WIN/Workaround: don't pass gid and uid to docker run call #140

Merged
merged 1 commit into from
Nov 10, 2020
Merged

Conversation

adswa
Copy link
Member

@adswa adswa commented Nov 10, 2020

A workaround in response to #138. The --user flag should now only be used when we're on a non-Windows OS.

@adswa
Copy link
Member Author

adswa commented Nov 10, 2020

Shall I cancel on of the CI runs?

@kyleam
Copy link
Contributor

kyleam commented Nov 10, 2020

Shall I cancel on of the CI runs?

Yes, please. It's happening because the push is to this repo rather than a fork. (I don't recall offhand whether there is a knob to prevent Travis from starting up both in this situation.)

"--rm"]
if not on_windows:
prefix_extend(["-u", "{}:{}".format(os.getuid(), os.getgid())])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/_extend/.extend/

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

argh, thanks for catching that! I hate notepad++. It so easy to lose/miss edits :/

The os module under Windows does not contain getuid() and getgid(), which
are called to supply a 'docker run' call with Unix user and group associations.
This PR is a temporary workaround in that 'docker run' is called without
the -u/--user flag entirely when executed under Windows.
@adswa
Copy link
Member Author

adswa commented Nov 10, 2020

Yes, please. It's happening because the push is to this repo rather than a fork. (I don't recall offhand whether there is a knob to prevent Travis from starting up both in this situation.)

Cancelled. I'll try to remember that and PR from a fork the next time.

Copy link
Contributor

@kyleam kyleam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

Fingers crossed that on your end you can at least do this now:

$ datalad containers-add --url dhub://busybox:1.30 bb
$ datalad containers-run -n bb sh -c 'busybox | head -1'
[INFO   ] Making sure inputs are available (this may take some time) 
[INFO   ] == Command start (output follows) ===== 
BusyBox v1.30.1 (2019-05-09 01:23:43 UTC) multi-call binary.
[INFO   ] == Command exit (modification check follows) ===== 
action summary:                                                                         
  get (notneeded: 1)
  save (notneeded: 1)

@adswa
Copy link
Member Author

adswa commented Nov 10, 2020

Success!

datalad@latitude-e7440 MINGW64 ~/repos/containers (adjusted/master(unlocked))
$ datalad containers-add --url dhub://busybox:1.30 bb
[INFO] Saved busybox:1.30 to C:\Users\datalad\repos\containers\.datalad\environments\bb\image
[INFO] Total: starting
[INFO]
[INFO] Total: processed result for C:\Users\datalad\repos\containers
[INFO] Total: done
add(ok): .datalad\environments\bb\image\64f5d945efcc0f39ab11b3cd4ba403cc9fefe1fa3613123ca016cf3708e8cafb.json (file)
add(ok): .datalad\environments\bb\image\a57c26390d4b78fd575fac72ed31f16a7a2fa3ebdccae4598513e8964dace9b2\VERSION (file)
add(ok): .datalad\environments\bb\image\a57c26390d4b78fd575fac72ed31f16a7a2fa3ebdccae4598513e8964dace9b2\json (file)
add(ok): .datalad\environments\bb\image\a57c26390d4b78fd575fac72ed31f16a7a2fa3ebdccae4598513e8964dace9b2\layer.tar (file)
add(ok): .datalad\environments\bb\image\manifest.json (file)
add(ok): .datalad\environments\bb\image\repositories (file)
add(ok): .datalad\config (file)
save(ok): . (dataset)
containers_add(ok): C:\Users\datalad\repos\containers\.datalad\environments\bb\image (file)
action summary:
  add (ok: 7)
  containers_add (ok: 1)
  save (ok: 1)

datalad@latitude-e7440 MINGW64 ~/repos/containers (adjusted/master(unlocked))
$ datalad containers-run -n bb sh -c 'busybox | head -1'
[INFO] Making sure inputs are available (this may take some time)
[INFO] == Command start (output follows) =====
BusyBox v1.30.1 (2019-05-09 01:23:43 UTC) multi-call binary.
[INFO] == Command exit (modification check follows) =====
[INFO] Total: starting
[INFO]
[INFO] Total: processed result for C:\Users\datalad\repos\containers
[INFO] Total: done
action summary:
  get (notneeded: 1)
  save (notneeded: 1)

@kyleam
Copy link
Contributor

kyleam commented Nov 10, 2020

Travis is running one job at a time, with lots of downtime in between: it's taken over 35 minutes to run two jobs whose total execution time is under 8 minutes and has yet to start the third job.

I think those two runs are sufficient for this change...

@kyleam kyleam merged commit 6c5a567 into master Nov 10, 2020
@kyleam kyleam deleted the win-138 branch November 10, 2020 21:15
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