search

soraxas / echo360

Commandline tool for automated downloads of echo360 videos hosted by university
https://cs.tinyiu.com/echo360
MIT License
261 stars 51 forks source link

Apple Silicon Support #76

Closed paulpall closed 9 months ago

paulpall commented 9 months ago

Changed the versions and suffixes to reflect the Apple Silicon drivers and updated downloader to work with newer Selenium.

soraxas commented 9 months ago

Hi @paulpall thank you for the PR!

What is the reason for the commented kwargs?

            #self._driver = webdriver.Chrome(**kwargs)
            self._driver = webdriver.Chrome()
soraxas commented 9 months ago

@paulpall I've updated your PR on actually detecting arm processor, to avoid breaking older mac that uses intel cpu. Can you verify if the new changes still work on your computer?

paulpall commented 9 months ago

Hi @paulpall thank you for the PR!

What is the reason for the commented kwargs?

            #self._driver = webdriver.Chrome(**kwargs)
            self._driver = webdriver.Chrome()

Honestly, the comments can be removed. The reason I didn't remove them was in case someone on a different platform ran into any issues, and because I didn't go through the codebase to see if kwargs were utilised elsewhere or if the firefox and chrome binary_downloader sections could be slimmed down.

paulpall commented 9 months ago

@paulpall I've updated your PR on actually detecting arm processor, to avoid breaking older mac that uses intel cpu. Can you verify if the new changes still work on your computer?

Nice! Yup, that still seems to works fine for me.

soraxas commented 9 months ago

Hi @paulpall thank you for the PR! What is the reason for the commented kwargs?

            #self._driver = webdriver.Chrome(**kwargs)
            self._driver = webdriver.Chrome()

Honestly, the comments can be removed. The reason I didn't remove them was in case someone on a different platform ran into any issues, and because I didn't go through the codebase to see if kwargs were utilised elsewhere or if the firefox and chrome binary_downloader sections could be slimmed down.

I don't quite understand what you meant; as in, was it that the original version with kwargs didn't work for you? Hence, you've commented them out?

soraxas commented 9 months ago

I'm asking that because they are used to set useragents on the browser, which was needed for the older (non-cloud) echo360 platform.

paulpall commented 9 months ago

Hi @paulpall thank you for the PR! What is the reason for the commented kwargs?

            #self._driver = webdriver.Chrome(**kwargs)
            self._driver = webdriver.Chrome()

Honestly, the comments can be removed. The reason I didn't remove them was in case someone on a different platform ran into any issues, and because I didn't go through the codebase to see if kwargs were utilised elsewhere or if the firefox and chrome binary_downloader sections could be slimmed down.

I don't quite understand what you meant; as in, was it that the original version with kwargs didn't work for you? Hence, you've commented them out?

Oh, yes it did not work, sorry. From what I could tell the executable_path was removed in Selenium 4.10.0 webdriver and is no longer required. However, it is possible to pass it as a Selenium service if required. Check this thread on Stack Overflow.

soraxas commented 9 months ago

In the requirements.txt there should be version specification on selenium: https://github.com/soraxas/echo360/blob/1d01827e444e0871cb6cb031078c8e22c92a25f8/requirements.txt#L2

paulpall commented 9 months ago

Interesting, let me do a bit of digging...

paulpall commented 9 months ago

In the requirements.txt there should be version specification on selenium:

https://github.com/soraxas/echo360/blob/1d01827e444e0871cb6cb031078c8e22c92a25f8/requirements.txt#L2

It seems that the requirements aren't working properly when installing with pip install echo360. I uninstalled everything and cleared the cache and still got Selenium 4.15.2. Screenshot of the installation below:

Screenshot 2023-12-04 at 10 54 58
soraxas commented 9 months ago

In the requirements.txt there should be version specification on selenium: https://github.com/soraxas/echo360/blob/1d01827e444e0871cb6cb031078c8e22c92a25f8/requirements.txt#L2

It seems that the requirements aren't working properly when installing with pip install echo360. I uninstalled everything and cleared the cache and still got Selenium 4.15.2. Screenshot of the installation below:

Screenshot 2023-12-04 at 10 54 58

I see! Thanks for confirming!

soraxas commented 9 months ago

The latest push should auto handle newer selenium version. It would be great if you can test it on your end

paulpall commented 9 months ago

The latest push should auto handle newer selenium version. It would be great if you can test it on your end

Unfortunately that's somehow trying to pass kwargs again, both with Firefox and Chrome. I will look into it a bit further to see why. Meanwhile I noticed that the package version at PyPI is out-of-date and that is probably the reason why it downloaded the latest Selenium for me.

paulpall commented 9 months ago

The latest push should auto handle newer selenium version. It would be great if you can test it on your end

Unfortunately that's somehow trying to pass kwargs again, both with Firefox and Chrome. I will look into it a bit further to see why. Meanwhile I noticed that the package version at PyPI is out-of-date and that is probably the reason why it downloaded the latest Selenium for me.

Never mind, I cleaned everything and ran it again, works great!

soraxas commented 9 months ago

Awesome! Thanks @paulpall for confirming! I'll merge the new changes and will also push to PyPI afterwards.