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

new: Implemented GitHub actions CI #174

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

PolpOnline
Copy link
Contributor

@PolpOnline PolpOnline commented Sep 5, 2021

Hi, I just implemented the GitHub actions CI to this repo.
IMO, that's better than Travis CI because its better integration with GitHub.
Now, there are two main problems with this pull request:

  • The Lint CI fails, it says basically that every file has styling issues (Here you can check the workflow run). I think that's Prettier's fault because whenever I tried to run yarn format on my ArchLinux machine and it gives me the same errors that the CI is giving. Maybe tweaking a bit with the .prettierrc configuration file can fix the issue.
  • The update_exiftool.pl doesn't check if the version installed of ExifTool is the latest, so every time the CI runs it has to download it from the exiftool.org site, I think that this is a useless bandwidth waste. My proposal to resolve this issue would be inserting some kind of check in the Perl script to test if the version downloaded is the latest and then cache the ExifTool files to reuse between one CI run and another (I already know how to implement the latter).
    Maybe I could try to implement a solution in the CI workflow but that would be more complicated than what I proposed (and also won't work if the runner isn't the CI).

Sorry for my bad English.

EDIT: nvm, I found what the first issue problem is, it's just that you used 4 spaces instead of a tab to format files, this is by default wrong for Prettier, just changing the .prettierrc file should be sufficient to tell Prettier not to substitute spaces with tabs. Can you tell me which indentation format do you prefer?

@szTheory
Copy link
Owner

szTheory commented Sep 6, 2021

Nice! I'm not familiar with Github Actions yet so it might take me a bit to learn Github Actions, and follow through and review this properly, but it looks good so far.

For the Prettier options, whatever runs and keeps the code the same without needing formatting commits works for me for now.

As for update_exiftool.pl, I like your idea. Maybe we just modify the Perl script to not delete the contents of DOWNLOADS_WORKING_DIR aka /exiftool_downloads when the script is done. Then if there is an existing file matching the SHA hash on the exiftool site, then download if it doesn't already exist. If that download dir is cached it will prevent the redundant downloads. Although we would still have to pull down the checksum file from the official site each time, that's just a short text file so it will go very fast.

How does that sound?

@szTheory szTheory added the devops label Sep 6, 2021
@PolpOnline
Copy link
Contributor Author

For the Prettier options, whatever runs and keeps the code the same without needing formatting commits works for me for now.

Ok, I'll fix that and open up a PR pointing to upstream, then, incorporate changes in this PR's branch.

Maybe we just modify the Perl script to not delete the contents of DOWNLOADS_WORKING_DIR aka /exiftool_downloads when the script is done. Then if there is an existing file matching the SHA hash on the exiftool site, then download if it doesn't already exist. If that download dir is cached it will prevent the redundant downloads. Although we would still have to pull down the checksum file from the official site each time, that's just a short text file so it will go very fast.

That sounds good, I don't know Perl, so I'll be glad if you can fix this on your master branch, then I can merge it to this pull request and implement caching of the file downloaded to this workflow.

@szTheory
Copy link
Owner

szTheory commented Sep 6, 2021

Yeah I update the Perl script for sure, maybe sometime this week

@PolpOnline PolpOnline changed the title New: Implemented GitHub actions CI new: Implemented GitHub actions CI Sep 13, 2021
@szTheory
Copy link
Owner

@PolpOnline Sorry for the delay. I added a --cache-downloads-working-dir option to update_exiftool.pl. So now you can run perl update_exiftool.pl --cache-downloads-working-dir and it will use the existing downloads, if they are there. Also merged in your Prettier fix #177

@PolpOnline
Copy link
Contributor Author

For some reason, the cache key doesn't have the hash of the files in there. However, this shouldn't be a problem anyway because the hashes of the files are checked by the Perl script.
So, I would consider this PR ready to be merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants