-
Notifications
You must be signed in to change notification settings - Fork 77
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
Issue 119: download files with different extension than exe #126
Conversation
I tried various combinations with this patch applied and all works great. @Nebelhom please add new tests to the manual list, so we can cover:
One idea I have is that we remove the ´(installer)?´ part from the regex and make it the default value of the extension option. That way it's better understandable. We might want to rename --extension to something better. All that would make it easier for us to get the stub installer part implemented. For downloading files like ´jsshell-win32.zip´ we should file a separate bug. |
With regard to the above commit, I have two comments:
compare:
etc.
|
'win32': 'exe', | ||
'win64': 'exe'} | ||
'win32': r'(\installer.)?exe', | ||
'win64': r'(\installer.)?exe'} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do not change those values here! ´installer´ should never be a part of it. This hash only contains real file extensions, which might be overwritten later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I must have misunderstood you then.
Quote:
"we remove the ´(installer)?´ part from the regex and make it the default value of the extension option."
I assumed you meant the default value for self.extension, which is marked in DEFAULT_FILE_EXTENSIONS
In fact,... where do you want me to move it then?...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please drop that part given that it doesn't apply anymore to any of your code in this PR. This was related to the other file types which has been spun off as a separate bug. With default I mean that when trying to download a file with a specific extension, the 'installer' string should be a default value for the file type, so that we automatically would grab installers by default. But anyway, it's not related to this pull.
See my other comment regarding the regex_suffix. I don't think that is true. Regarding the --extension flag we agreed on IRC that we want to move all that to a separate issue. This PR only cares about the real file extension. Releases don't have zip files, that's correct. So it only applies to daily and tinderbox builds. |
@@ -27,6 +27,7 @@ mozdownload -a firefox -t candidate -p win32 -v 21.0 | |||
mozdownload -a firefox -t candidate -p win32 -v 21.0 -d firefox-candidate-builds | |||
mozdownload -a firefox -t candidate -p win32 -v 21.0 -l cs | |||
mozdownload -a firefox -t candidate -p win32 -v 21.0 -l en-GB | |||
mozdownload -a firefox -t candidate -p win32 -v 21.0 --extension=zip |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Could we move this up a few lines to match the order of the other sections?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, please use txt as the extension as it's much quicker for manual testing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check whimboo's comment regarding zip archives of Firefox on Windows ( #126 (comment) )
I assumed, we want to explicitly test for the zip files.
Am I interpreting it correctly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It depends, really here we're testing that you can specify an alternate extension... we're not really interested in testing that the appropriate files are available for download. If that were to fail it would likely be a release engineering bug and not a mozdownload one.
This PR has been bitrotted. Can you please update the patch so I can have a quick spot-check for its functionality? |
@whimboo It should be good to go now... |
Sorry, but this does not work:
It should not download the crash symbols but the build. Someone similar to daily builds (tinderbox might also be affected)
Why doesn't it find the folder? I think we have to skip this feature from 1.9 and move it to 1.10. |
fyi: The reason the crashreporter-symbols.zip is download is, because there is more than one zip file in the folder and crashreporter-symbols happens to be the first in the list. I'll try to catch you on IRC some time this week and see if we can come up with an elegant solution. Everything I can think of tonight feels hackish. |
As of now we should only take zip files which also start with the name of the application specified. So we shouldn't get crashreporter-* files at all. |
The Note the The specific lines for https://github.com/mozilla/mozdownload/blob/master/mozdownload/scraper.py#l442 |
So as given on bug 922241 zip archives of Firefox will be removed soon. So would that invalidate this PR? |
It does not necessarily invalidate the PR. We are allowing for the absence or presence of the 'installer' portion in the filename. Having looked at a tinderbox build we still have the following file formats without the installer part in the name:
These could still be downloaded with mozdownload if that is desireable. If not, we should scrap this PR |
Sorry for taking so long with my reply! It is! At least for the .json and .txt files. Those are not builds but could be of good use for us. So lets the PR updated. |
I updated it now and changed the manual test cases from zip to json and txt as test. Apologies for the empty merge commit, but rebasing from the mozilla/mozdownload repository kept on inserting jarekps' last PR of the dailyscraper tests in the middle of my commits. Quite annoying... I hope it is ok like that too |
Looks good, and tests well for me. Do you want to have a final look over @whimboo or shall I merge this? |
'win32': r'(\.installer)%(STUB)s\.%(EXT)s$', | ||
'win64': r'(\.installer)%(STUB)s\.%(EXT)s$'} | ||
'win32': r'(\.installer)?%(STUB)s\.%(EXT)s$', | ||
'win64': r'(\.installer)?%(STUB)s\.%(EXT)s$'} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the stub part only exists if we have an installer. So we might want to make the whole part optional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reply here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh sorry. I wanted to answer, but must have forgotten. I think it makes sense. Once the tests are in, I will move the stub into the brackets, as well.
I don't know about your current version of mozdownload, but the presence of a stub installer in the win32 version causes all tinderbox_scraper tests with the flag I need to go and fix Issue #180 first, so that those tests work again, otherwise I can't be certain that my current changes work reliably. Sorry for the mess... |
@@ -3,3 +3,29 @@ | |||
build | |||
dist | |||
tests/venv | |||
run_tests.sh~ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmmm... I didn't mean to put this in, but it might as well be. It is a constant source of aggravation for me. If I should remove it let me please know. thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove and adjust your editor to not create those backups for the mozdownload project.
Ok, next try. Sorry for the many commits. I forgot to clean up after myself (blush). The tests run through ok and seems ok. @whimboo: could you please review? Thanks a lot. |
@@ -0,0 +1,11 @@ | |||
Lorem ipsum dolor sit amet, consectetuer adipiscing elit. Nam cursus. Morbi ut mi. Nullam enim leo, egestas id, condimentum at, laoreet mattis, massa. Sed eleifend nonummy diam. Praesent mauris ante, elementum et, bibendum at, posuere sit amet, nibh. Duis tincidunt lectus quis dui viverra vestibulum. Suspendisse vulputate aliquam dui. Nulla elementum dui ut augue. Aliquam vehicula mi at mauris. Maecenas placerat, nisl at consequat rhoncus, sem nunc gravida justo, quis eleifend arcu velit quis lacus. Morbi magna magna, tincidunt a, mattis non, imperdiet vitae, tellus. Sed odio est, auctor ac, sollicitudin in, consequat vitae, orci. Fusce id felis. Vivamus sollicitudin metus eget eros. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer a .txt file to download. We can make use of the ones in the latest sub folders.
next try. thx for your patience |
This looks good to me. I will get it merged in a bit. |
Landed as 1ca4bbd |
This pull addresses Issue #119
Not much to it really.