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

Add SINGLEFILE_ARGS to control single-file arguments #1021

Merged
merged 3 commits into from
Jan 10, 2023

Conversation

notevenaperson
Copy link
Contributor

@notevenaperson notevenaperson commented Sep 11, 2022

Making a pull request of @renaisun's work for them, since it looks good to me but has been cold for two months.

Summary

#981

Changes these areas

  • Bugfixes
  • Feature behavior
  • Command line interface
  • Configuration options
  • Internal architecture
  • Snapshot data layout on disk

@renaisun
Copy link
Contributor

renaisun commented Sep 11, 2022

@notevenaperson Hi, I found this fix is not well rounded. SingleFile need a browser to run which could be set via --browser-executable-path. But for archivebox, the browser path is set and passed to singlefile-cli via CHROME_BINARY, which seems ambiguous and redundant. See this
Should it be removed?

@notevenaperson
Copy link
Contributor Author

@notevenaperson Hi, I found this fix is not well rounded. SingleFile need a browser to run which could be set via --browser-executable-path. But for archivebox, the browser path is set and passed to singlefile-cli via CHROME_BINARY, which seems ambiguous redundant. See this Should it be removed?

IMO that's not an issue. For example COOKIES_FILE affects wget, but user can still redundantly set WGET_ARGS=["--load-cookies=cookies.txt"]. If WGET_ARGS is okay SINGLEFILE_ARGS also is. It's up to the user to sort these redundancies in his or her own config.

@renaisun
Copy link
Contributor

@notevenaperson Hi, I found this fix is not well rounded. SingleFile need a browser to run which could be set via --browser-executable-path. But for archivebox, the browser path is set and passed to singlefile-cli via CHROME_BINARY, which seems ambiguous redundant. See this Should it be removed?

IMO that's not an issue. For example COOKIES_FILE affects wget, but user can still redundantly set WGET_ARGS=["--load-cookies=cookies.txt"]. If WGET_ARGS is okay SINGLEFILE_ARGS also is. It's up to the user to sort these redundancies in his or her own config.

Sorry for the confusion. I think there shouldn't be --browser-executable-path outside the SINGLEFILE_ARGS.

Otherwise, the --browser-executable-path in SINGLEFILE_ARGS will be overwritten once the user set CHROME_BINARY. I'm not sure if there is any guarantee that the arguments are read sequentially.

@notevenaperson
Copy link
Contributor Author

notevenaperson commented Sep 11, 2022

Oh I understand now. If the user sets SINGLEFILE_ARGS=["--browser-executable-path=foobar"], single-file would be given the --browser-executable-path two times. I tested it and it does cause an error:

single-file --browser-executable-path=foobar --browser-executable-path=/bin/chromium https://example.com test.html
The "file" argument must be of type string. Received an instance of Array

This pull request should also have code to eliminate conflicting (duplicated) options in the final command.

@notevenaperson
Copy link
Contributor Author

notevenaperson commented Sep 11, 2022

@renaisun I sent a pull request to fix the issue. Thanks for bringing it up. If you accept the pull request, it'll be added here.

renaisun/ArchiveBox/pull/1

@renaisun
Copy link
Contributor

@renaisun I sent a pull request to fix the issue. Thanks for bringing it up. If you accept the pull request, it'll be added here.

renaisun/ArchiveBox/pull/1

Thanks for your work. Let's see if the PR will be merged🤣

@pirate
Copy link
Member

pirate commented Sep 22, 2022

Thanks for your work here! Will review this soon.

@tzwm
Copy link

tzwm commented Jan 2, 2023

@pirate please review this issue. I really want to add more args to SingleFile. Thanks!

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

Successfully merging this pull request may close these issues.

4 participants