8000 Boutiques on Windows by arthursw · Pull Request #561 · boutiques/boutiques · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Boutiques on Windows #561

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
Merged
merged 10 commits into from
Jan 15, 2020
Merged

Boutiques on Windows #561

merged 10 commits into from
Jan 15, 2020

Conversation

arthursw
Copy link
Contributor
@arthursw arthursw commented Dec 3, 2019 •
edited by glatard
Loading

name: Fix windows issues (paths and file names)
about: Remove colon in file names (invalid characters in windows), and make path homogeneous among platforms.
title: 'Windows bug fix'
labels: windows support
assignees: ''

Checklist

  • DO Unit tests pass.

For now unit tests do not pass because of the error: DescriptorValidationError: [ ERROR ] Additional properties are not allowed ('deprecated' was unexpected) (some descriptors seem out of date?)

Purpose

This PR makes paths homogeneous among platforms (tested on windows and linux only but should work on mac as well).

When used in file names,datetime.datetime.now().isoformat() is replaced by datetime.datetime.now().strftime("%Y-%m-%d_%Hh%Mm%Ss%fms") to avoid the colon (:) character which causes problem on Windows.

The command type seems to have a different purpose on Windows so to check if Singularity or Docker is installed the command docker --version or singularity --version is called instead.
I had to remove "&>/dev/null" from the command line 542 of localExec.py.

Current behaviour

Works on Linux and Windows.

New behaviour

Now also works on windows.

Does this introduce a major change?

  • Yes
  • No

@glatard
Copy link
Member
glatard commented Jan 13, 2020

Hi @arthursw, thank you so much for working on this! I noticed that you added several JSON or txt files that aren't part of the code, probably because they were generated by the tests: do you mind removing them? It might be the reason why the tests are failing.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.7%) to 94.386% when pulling d065ea1 on arthursw:master into 01696d5 on boutiques:develop.

@arthursw
Copy link
Contributor Author

@glatard ok, I removed the test files ; sorry about that!

@glatard
Copy link
Member
glatard commented Jan 15, 2020 •
edited
Loading

The tests are now passing and the code makes a lot of sense to me. I'm just surprised that we don't have more Linux-specific things (like '\n' or manual path concatenations) elsewhere in the code! I have started looking at Appveyor config to test the code on Windows (see #445) but didn't manage to have it working yet. As these modifications don't seem to break anything on Linux, I'd suggest we merge them and then work on Windows testing through Appveyor. I'd like to have approval from @gkiar or @ramou first though.

@arthurmasson, if you have any experience with Appveyor, your help would be greatly appreciated!

glatard
glatard approved these changes Jan 15, 2020
View reviewed changes
Copy link
Member
@glatard glatard left a comment

Choose a reason for hiding this comment

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

Looks very good to me! Given that the work on paths and mounts has created issues in the past, I'd like to have approval from @gkiar or @ramou before we merge this.

@ramou
Copy link
Contributor
ramou commented Jan 15, 2020 via email

gkiar
gkiar approved these changes Jan 15, 2020
View reviewed changes
Copy link
Member
@gkiar gkiar left a comment

Choose a reason for hiding this comment

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

It looks good to me, thanks @arthursw ! 👍

@glatard glatard merged commit 17eb41e into boutiques:develop Jan 15, 2020
@arthursw
Copy link
Contributor Author

Actually, I didn't check that every features work on windows... I just focused on what is required for the medInria plugin. I don't have any experience with Appveyor but I can have a try if you need it.

@glatard glatard mentioned this pull request Mar 17, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gkiar gkiar gkiar approved these changes

@glatard glatard glatard approved these changes

Assignees

@gkiar gkiar

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@arthursw @glatard @coveralls @ramou @gkiar @arthurmasson
0