-
Notifications
You must be signed in to change notification settings - Fork 162
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
Allow overriding web bundle at _build time_ #1131
base: master
Are you sure you want to change the base?
Conversation
cdfc8fa
to
2967a30
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1131 +/- ##
=======================================
Coverage 46.92% 46.92%
=======================================
Files 177 177
Lines 65160 65165 +5
=======================================
+ Hits 30575 30580 +5
Misses 34585 34585 ☔ View full report in Codecov by Sentry. |
2967a30
to
4491480
Compare
This is somewhat related to #1121, which completely overhauls Tectonic's bundles (and thus conflicts with the changes here) Notably, #1121 adds a new bundle format, and setting a variable for just the url prefix won't work. It may be a better idea (even without extra bundles) to have the variable contain the whole URL. Also, I don't think I understand the difference between the LOCKED and PREFIX variables. Is there any reason to have two? I think one variable that overrides the default bundle url should be enough. A few more notes about the issue in #1002:
|
Hi @rm-dr! Thank you for the comment!
Wow amazing! I didn't know this before! This is a huge change though. Are you aware of #1122? I think being a member would probably make it easier to land these changes.
Currently, the bundle url depends on the tectonic/crates/bundles/src/lib.rs Lines 113 to 123 in c64e524
So the PREFIX variant only modifies the prefix, in this case https://relay.fullyjustified.net , and keeps the version suffix default_bundle_v{format_version}.tar . If this is going away in the future, then only the LOCKED variant is needed. What do you think? Should I simply remove the PREFIX variant?
They are different! Let me clarify:
I think for the runtime override, we should only provide the CLI option, because env variables are more implicit and thus reduce reproducibility. For build time, however, I think env variables are the only way to pass around the customized build constants, so I have to use it.
AFAIK,
I think this behavior is nice as it maximizes reproducibility. There are some suggestions in issues to put the locked url $ tectonic -X init
note: "version 2" Tectonic command-line interface activated
note: creating new document in this directory ($HOME/apps/tectest)
note: connecting to https://relay.fullyjustified.net/default_bundle_v33.tar
note: resolved to https://data1.fullyjustified.net/tlextras-2022.0r0.tar
$ cat Tectonic.toml
[doc]
name = "tectest"
bundle = "https://data1.fullyjustified.net/tlextras-2022.0r0.tar"
... Note how the url gets redirected and locked down. |
I really like the idea of having an environment variable for this, but I also don't understand why we need 2 environment variables, and not only 1. Besides that, I also support having this environment variable read in runtime and not build time, as this would also help users to test different bundles in a different way, I think there is no advantage in terms of reproducibility between reading this environment variable in runtime vs build time; since Tectonic from different distributions may have different environments anywhere. I also think it'd be fair for Tectonic to not guarantee reproducibility in case it reads a dedicated environment variable, and there's a conflict with the URL in the |
Hi @doronbehar! Thank you very much for the comment!
Yes, this is true in reality, but ideally, I would like Tectonic to not read any environment variables at all (at runtime), by design. It's like the new flake-y nix no longer reads the I know @pkgw is probably busy, but since this is now more than a bugfix and related to the UI design, I would very much like to hear your opinion, and I can fix the code accordingly! |
I did noticed that the 2nd commit of this PR as of currently is referring to this? But not completely? Maybe it'd be easier for upstream to digest this change only in a PR of its own. |
Ah, I see. We really do need a compile-time option for this. and I don't think I know a better way to do this, if we want easy bundle updates through a "default" redirect. |
Following the suggestion of @doronbehar I have moved the CLI changes to a separate PR #1132. These two PRs are both related to the web bundle interface, but they are technically independent, so it would be easier to review this way. |
3056a01
to
4491480
Compare
I've merged #1132, so that this PR becomes a lot smaller. It does seem reasonable to me to have the default bundle URL be build-time configurable, for specialized applications. My only comment upon first glance is one of the evergreen ones: these changes should come with corresponding documentation somewhere! |
Yes indeed! But since #1132 (run time web bundle override) has been merged, this one is no longer a blocking issue for me 😆 But I will try to document it sometime! |
4491480
to
e2fcb53
Compare
e2fcb53
to
489dfb2
Compare
489dfb2
to
0cb32d3
Compare
This commit makes it easy to override the default web bundle through the environment variables: - `TECTONIC_WEB_BUNDLE_PREFIX` - `TECTONIC_WEB_BUNDLE_LOCKED` The PREFIX variable makes it easy for people to track a mirrored bundle, while the LOCKED variable ensures reproducible builds across all CLIs.
0cb32d3
to
35c3664
Compare
Note: I would like to postpone the work on the missing documentations, until the monumental bundle rework has settled down, and then I will be rebasing this as soon as possible. Thank you all very much for maintaining tectonic!
This PR enables us to override the default web bundle at build time, through the environment variables (or in
build.rs
):TECTONIC_WEB_BUNDLE_PREFIX
TECTONIC_WEB_BUNDLE_LOCKED
The PREFIX variable makes it easy for people to track a mirrored bundle, while the LOCKED variable ensures reproducible builds across all CLIs. This is related to #1002 and its resolution in #1132.
Currently, the default web bundle is hardcoded, as observed in #1002. A related issue is the version mismatch between biber and the tectonic bundle #893, which we
would like toaddressed in #1132 and nixpkgs NixOS/nixpkgs#273740.P.S. I am very new to rust, so if the code / interface is no good, please help me improve! And thank you for this wonderful package!