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

Fix \iow_open:N in ConTeXt MkII #1114

Merged
merged 5 commits into from Jul 22, 2022
Merged

Conversation

Witiko
Copy link
Contributor

@Witiko Witiko commented Jul 14, 2022

For ConTeXt MkII, we have to deal with the fact that \newwrite works like our own: it actually checks before altering definition. This pull request patches \__iow_new:N accordingly. See Witiko/lt3luabridge#3 for a discussion of the issue with @josephwright.

l3kernel/l3file.dtx Outdated Show resolved Hide resolved
l3kernel/l3file.dtx Outdated Show resolved Hide resolved
@josephwright
Copy link
Member

Please can something go into the ChangeLog too

@Witiko
Copy link
Contributor Author

Witiko commented Jul 15, 2022

It would be good to also run automated tests for ConTeXt MkII, so that we don't do these fixes blindly and know when things break. From my discussion with @josephwright in Witiko/lt3luabridge#3, I understand that we only check ConTeXt MkIV and later at the moment.

@Witiko
Copy link
Contributor Author

Witiko commented Jul 15, 2022

@josephwright @FrankMittelbach Thank you for your reviews. I am coding on my phone over SSH from vacation, which is not very ergonomic, so I will do the finishing touches next week when I am in the office. I also welcome commits to this PR.

@u-fischer
Copy link
Member

It would be good to also run automated tests for ConTeXt MkII,

I don't think that it make sense to spent time to setup tests for MkII in the LaTeX repo. As Joseph said, MkII is two generations behind.

ConTeXt defines an entire suite of \normal... commands. If we wanted to be extra careful and make sure that the user didn't just happen to define \normalend, we could check that all of them are defined. Such a test would be best wrapped into a function \sys_if_format_context:TF or similar, because it would be tedious to repeat and prone to inconsistencies.

Imho it would be better to ask context which test can be used and will stay stable so that we don't have to adjust all the time.

@Witiko
Copy link
Contributor Author

Witiko commented Jul 15, 2022

@u-fischer Thank you for your review.

I don't think that it make sense to spent time to setup tests for MkII in the LaTeX repo

Such a setup should amount to one line in .github/workflows/*.yml, which would install Ruby. Admittedly, I don't know how well-suited l3build is to testing other formats; do we currently test ConTeXt at all? If we do, MkII is just a different command: texexec instead of context. See the continuous integration of lt3luabridge for an example.

which test can be used and will stay stable so that we don't have to adjust all the time.

That's just one concern. The other concern are false positives, which can result from users defining a command named \normalend, see #1114 (comment). Here, it makes sense to check several commands, so that this risk is minimized.

@josephwright
Copy link
Member

@u-fischer Thank you for your review.

I don't think that it make sense to spent time to setup tests for MkII in the LaTeX repo

Such a setup should amount to one line in .github/workflows/*.yml, which would install Ruby. Admittedly, I don't know how well-suited l3build is to testing other formats; do we currently test ConTeXt at all?

I have checked with ConTeXt a few times, and I suspect that MkII should be fine. However, the issue is that it's not 'a one line change'. To use the tests locally, MkII requires Ruby, etc. I've no idea how easy that is on macOS (my setup), but I know it's non-trivial on Windows. In particular, MkII doesn't run with 'OS + TeX Live', which is what we typically expect.

@josephwright
Copy link
Member

I suspect the best marker for ConTeXt is \starttext. l3names uses \normalend as it's saving the \end primitive, so there is a direct link, but that's not the case for wider usage.

@josephwright
Copy link
Member

@u-fischer Thank you for your review.

I don't think that it make sense to spent time to setup tests for MkII in the LaTeX repo

Such a setup should amount to one line in .github/workflows/*.yml, which would install Ruby. Admittedly, I don't know how well-suited l3build is to testing other formats; do we currently test ConTeXt at all?

I have checked with ConTeXt a few times, and I suspect that MkII should be fine. However, the issue is that it's not 'a one line change'. To use the tests locally, MkII requires Ruby, etc. I've no idea how easy that is on macOS (my setup), but I know it's non-trivial on Windows. In particular, MkII doesn't run with 'OS + TeX Live', which is what we typically expect.

(I did check I think once with MkII, but mainly checked against MkIV as that's a lot easier to use - at the cost that an up-to-date setup might not align with TeX Live.)

@Witiko
Copy link
Contributor Author

Witiko commented Jul 15, 2022

I suspect the best marker for ConTeXt is \starttext.

That is true, but it is as likely as \normalend to be defined as a user command, if not moreso.

Ideally, we would check some internal ConTeXt command that contains characters that are not letter in the normal regime, such as @ in LaTeX2e.

Failing that, we may want to check for the existence of several commands and require that they are all defined for robustness against user commands. \starttext may be one of these.

It may be easiest to just ask with the ConTeXt mailing list as @u-fischer suggested.

@josephwright
Copy link
Member

I suspect the best marker for ConTeXt is \starttext.

That is true, but it is as likely as \normalend to be defined as a user command, if not moreso. Ideally, we would check some internal ConTeXt command that contains characters that are not letter in the normal regime, such as @ in LaTeX2e. Failing that, we may want to check for the existence of several commands and require that they are all defined for robustness against user commands.

A typical test for LaTeX2e is \documentclass, so I'm not sure that's a major concern.

@Witiko
Copy link
Contributor Author

Witiko commented Jul 15, 2022

A typical test for LaTeX2e is \documentclass, so I'm not sure that's a major concern.

In expl3 code? That does not seem safe. We may want to extract this into a separate testing function (\sys_if_format_latex:TF or similar), so that we can harden the test if needed and ensure consistency across the code base. However, this seems out of scope for this PR.

@Witiko
Copy link
Contributor Author

Witiko commented Jul 15, 2022

To use the tests locally, MkII requires Ruby, etc. I've no idea how easy that is on macOS (my setup), but I know it's non-trivial on Windows. In particular, MkII doesn't run with 'OS + TeX Live', which is what we typically expect.

If that is a concern, could we mark the ConTeXt MkII tests as extra and not run them by default, only in the CI? It's also OK to only check ConTeXt in downstream packages such as lt3luabridge and markdown if you'd rather like to keep the focus here on LaTeX.

@josephwright
Copy link
Member

A typical test for LaTeX2e is \documentclass, so I'm not sure that's a major concern.

In expl3 code? That does not seem safe.

No, I meant generally: it's often used to see if you need to load e.g. miniltx.

@josephwright
Copy link
Member

To use the tests locally, MkII requires Ruby, etc. I've no idea how easy that is on macOS (my setup), but I know it's non-trivial on Windows. In particular, MkII doesn't run with 'OS + TeX Live', which is what we typically expect.

If that is a concern, could we mark the ConTeXt MkII tests as extra and not run them by default, only in the CI? It's also OK to only check ConTeXt in downstream packages such as lt3luabridge and markdown if you'd rather like to keep the focus here on LaTeX.

Certainly doable but like I said, ConTeXt MkII has been obsolete for about 15 years and ConTeXt really doesn't work like plain or LaTeX, in the sense that most ConTeXt users do update their sources as a matter of routine.

@Witiko
Copy link
Contributor Author

Witiko commented Jul 15, 2022

No, I meant generally: it's often used to see if you need to load e.g. miniltx.

As long as that happens outside the kernel code, that does not seem like a concern. However, if l3sys can provide a standard function to detect the format, that might also improve the situation on the package side of things.

@u-fischer
Copy link
Member

Such a setup should amount to one line in .github/workflows/*.yml

it is not enough to setup a test workflow in github. Someone will also have to write test files (and test them locally), and debug them if they fail. I have no idea how to setup and run such a context, and I don't want to have to spent hours to get it working. If something fails in MkII and someone is interested enough to provide code to correct this, we can try to add it. But we should not make the impression that we committed ourselves to support MkII by setting up a dedicated test suite in the latex repo.

@Witiko
Copy link
Contributor Author

Witiko commented Jul 15, 2022

The consensus seems to be that we don't want to test ConTeXt MkIi here.

@Witiko Witiko marked this pull request as draft July 15, 2022 19:42
@Witiko
Copy link
Contributor Author

Witiko commented Jul 16, 2022

Hans Hagen and Henri Menke suggest we use \contextversion to detect ConTeXt. This command should be stable, reducing the chance of false negatives. Like \normalend, \contextversion is also all letters, but it says context and therefore seems less likely to be defined by a user, reducing the chance of false positives.

@FrankMittelbach
Copy link
Member

Hans Hagen suggests we use \contextversion to detect ConTeXt.

a very good suggestion. All letters is fine given that "context" is part of the name. I think we should use that throughout.

@Witiko
Copy link
Contributor Author

Witiko commented Jul 21, 2022

Ah, but we already have \c_sys_engine_format_str, which outputs cont-en for all versions of ConTeXt, I should have noticed earlier. Perhaps we can just use that instead of adding yet another variable/conditional. I am discussing this with ConTeXt developers at the dev-context mailing list.

@Witiko Witiko marked this pull request as ready for review July 21, 2022 19:37
@josephwright josephwright merged commit bb49853 into latex3:main Jul 22, 2022
Copy link
Member

@FrankMittelbach FrankMittelbach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no issues with the changes as such based on what was there before, but I think that from a conceptional point of view we should not put conditional code into macros that are conditional based on the engine/format in use.

Instead the conditional should be outside and macros should have different definitions based on the given engine/format.
In this particular situation it doesn't make much of a difference I guess, but in my opinion we should always provide code in the way that such conditionals are not executed more often than necessary, i.e., not during runtime. I guess that should go into a separate pr to be resolved over time (there may be other places).

@josephwright
Copy link
Member

@FrankMittelbach I've merged the PR and will tidy up the conditionals :)

@josephwright
Copy link
Member

@FrankMittelbach I've looked here: everything seems to be conditional during loading but not during usage - I think we are OK.

@Witiko
Copy link
Contributor Author

Witiko commented Jul 25, 2022

@josephwright @FrankMittelbach Hans Hagen and Mojca Miklavec note that detecting ConTeXt using cont-en does not account for the fact that different user interfaces will have different suffixes such as cont-nl for Dutch, which Mojca says has been ``widely used in the past''. Maybe we should:

  • Replace \str_if_eq:VnTF \c_sys_engine_format_str { cont-en } for \cs_if_exist:NTF \contextversion:

\str_if_eq:VnTF \c_sys_engine_format_str { cont-en }

\str_if_eq:VnT \c_sys_engine_format_str { cont-en }

\str_if_eq:VnTF \c_sys_engine_format_str { cont-en }

\str_if_eq:VnT \c_sys_engine_format_str { cont-en }

  • Update l3sys.dtx documentation, so that it cannot be misread as saying that ConTeXt always has \fmtname of cont-en:

latex3/l3kernel/l3sys.dtx

Lines 114 to 122 in cd8b163

% \begin{variable}[added = 2020-08-20]{\c_sys_engine_format_str}
% The name of the preloaded format for the current \TeX{} run given
% as a lower case string: one of
% |lualatex| (or |dvilualatex|),
% |pdflatex| (or |latex|), |platex|, |uplatex| or |xelatex| for \LaTeX{},
% similar names for plain \TeX{} (except \pdfTeX{} in DVI mode yields
% |etex|), and |cont-en| for Con\TeX{}t (i.e.~the
% \tn{fmtname}).
% \end{variable}

I would like to add a branching conditional or a predicate to l3sys.dtx (or l3candidates.dtx) for detecting ConTeXt using \cs_if_exist:NTF \contextversion, but it is not yet clear to me how to do that in a way that makes the conditional available in l3file.dtx during loading.

@josephwright
Copy link
Member

I would like to add a branching conditional or a predicate to l3sys.dtx (or l3candidates.dtx) for detecting ConTeXt using \cs_if_exist:NTF \contextversion, but it is not yet clear to me how to do that in a way that makes the conditional available in l3file.dtx during loading.

We don't have expl3 interfaces for detecting the format, and I don't think we want one: you test for features, not the format per se, if you need to do that.

@josephwright
Copy link
Member

  • Update l3sys.dtx documentation, so that it cannot be misread as saying that ConTeXt always has \fmtname of cont-en:

I'd be minded to force cont-en irrespective of the -<lang> part, as in 'cont-en is ConTeXt from POV'. I'm back with the point that today the number of users of MkII or even MkIV is pretty limited - I simply don't see someone using MkII for a historical document wanting to load expl3.

@Witiko
Copy link
Contributor Author

Witiko commented Jul 25, 2022

We don't have expl3 interfaces for detecting the format, and I don't think we want one: you test for features, not the format per se, if you need to do that.

Can we do that in l3file.dtx? It seems to me that we have to test for the format per se there.

I'd be minded to force cont-en irrespective of the - part, as in 'cont-en is ConTeXt from POV'.

As in force \c_sys_engine_format_str to say cont-en whenever \fmtname is cont-*? That seems fine by me. However, @u-fischer suggested that we ask ConTeXt what a robust test would be and ConTeXt devs say that we should use \contextversion rather than \fmtname.

@josephwright
Copy link
Member

We don't have expl3 interfaces for detecting the format, and I don't think we want one: you test for features, not the format per se, if you need to do that.

Can we do that in l3file.dtx? It seems to me that we have to test for the format per se there.

Yes, at the point of loading stuff one has to check on this, but I meant as a public interface.

I'd be minded to force cont-en irrespective of the - part, as in 'cont-en is ConTeXt from POV'.

As in force \c_sys_engine_format_str to say cont-en whenever \fmtname is cont-*? That seems fine by me. However, @u-fischer suggested that we ask ConTeXt what a robust test would be and ConTeXt devs say that we should use \contextversion rather than \fmtname.

We can use whatever: it's not really a major problem.

@Witiko
Copy link
Contributor Author

Witiko commented Jul 25, 2022

We can use whatever: it's not really a major problem.

Regardless, replacing the \fmtname test with the \contextversion test seems as a benefit (however minor), so I opened #1117.

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.

None yet

4 participants