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

Fixes in download-file script for windows #318

Merged
merged 5 commits into from
Oct 2, 2024

Conversation

anandhu-eng
Copy link
Contributor

No description provided.

@anandhu-eng anandhu-eng requested a review from a team as a code owner October 1, 2024 13:57
Copy link

github-actions bot commented Oct 1, 2024

MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅

@@ -207,7 +212,6 @@ def preprocess(i):
env['CM_PRE_DOWNLOAD_CMD'] = ''

if os_info['platform'] == 'windows' and env.get('CM_DOWNLOAD_CMD', '') != '':
env['CM_DOWNLOAD_CMD'] = env['CM_DOWNLOAD_CMD'].replace('&', '^&').replace('|', '^|').replace('(', '^(').replace(')', '^)')
Copy link
Contributor

Choose a reason for hiding this comment

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

don't we need this replace?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tested it on both powershell and command prompt. Both worked fine only when the escaping was removed.

Copy link
Contributor

Choose a reason for hiding this comment

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

@anandhu-eng - may I suggest you to add # to this line instead of removing. Probably there was a reason why it was added. If we encounter an issue in the future, we can then trace it back ... Thanks a lot!!!

Copy link
Contributor

Choose a reason for hiding this comment

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

I will then merge it and test on my machine before running another PR ...

Copy link
Contributor

Choose a reason for hiding this comment

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

@anandhu-eng It broke the Windows test of ABTF POC - it was working fine earlier as it uses wget for the download I believe.

https://github.com/mlcommons/cm4mlops/actions/runs/11126891963/job/30917906688?pr=318#step:5:3440

@anandhu-eng anandhu-eng marked this pull request as draft October 1, 2024 14:10
if tool != "rclone":
text = text.replace('%', "%%")

print(text)
Copy link
Contributor

Choose a reason for hiding this comment

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

we don't need the print right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi, cleaned in commit b522d7e

@anandhu-eng anandhu-eng marked this pull request as ready for review October 1, 2024 19:23
Copy link
Contributor

@gfursin gfursin left a comment

Choose a reason for hiding this comment

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

Thank you @anandhu-eng

@gfursin gfursin merged commit b0a217e into mlcommons:main Oct 2, 2024
31 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Oct 2, 2024
@anandhu-eng anandhu-eng deleted the downloadfilefix branch October 3, 2024 04:47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants