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

Incompatibility with package `scrbase` #7

Closed
mrpiggi opened this issue Nov 4, 2019 · 34 comments

Comments

@mrpiggi
Copy link
Contributor

@mrpiggi mrpiggi commented Nov 4, 2019

The package scrbase already provides both \ifpdftex and \ifVTeX for a very long time. The current update of iftex potentially raises a lot of problems with existing packages and documents.

\documentclass{article}
\usepackage{scrbase}
\usepackage[T1]{fontenc}
%\usepackage{iftex}[2019/10/24]
\begin{document}
\meaning\ifpdftex

\meaning\ifVTeX
\end{document}

Edit:
It may be better to define a new command \ifengine{engine-string}{true-branch}{false-branch} with the well-known LaTeX syntax and to provide the origin TeX switches only when the outdated packages (ifpdftex, ifluatex, ifxetex etc.) are loaded, whereas those should give a warning or at least an information that from now on package iftex should be used. With this approach, it wouldn't be necessary to provide switches in different letter-cases. I thought about something like this:

\def\IFTEX@let#1#2{%
  \expandafter\let\csname IFTEX@#1\expandafter\endcsname
  \csname if#2\endcsname}

\newcommand*\ifengine[1]{%
  \begingroup%
    \edef\reserved@a{\lowercase{\def\noexpand\reserved@a{#1}}}\reserved@a%
    \expandafter\ifx\csname IFTEX@\reserved@a\endcsname\relax%
      % ERROR or \aftergroup\@secondoftwo%
    \else%
      \csname IFTEX@\reserved@a\endcsname%
        \aftergroup\@firstoftwo%
      \else%
        \aftergroup\@secondoftwo%
      \fi%
    \fi%
  \endgroup%
}


\ifengine{pdftex}{true-branch}{false-branch}
\ifengine{pdfTeX}{true-branch}{false-branch}
\ifengine{PDFTeX}{true-branch}{false-branch}
@josephwright

This comment has been minimized.

Copy link
Member

@josephwright josephwright commented Nov 4, 2019

You'd need all the possible variations, or use something much more 'clever' for case changing, as the tests you've suggested are not expandable.

@davidcarlisle

This comment has been minimized.

Copy link
Member

@davidcarlisle davidcarlisle commented Nov 4, 2019

There is really a very widespread convention (but sadly not universal) that commands starting \if.. are tex primitive \if with a matching \fi

The collection of names in the new iftex is (apart for iftutex and ifluahbtex) taken from existing packages, and then some normalisation of mixed or lower case names.

That said it would be good not to break documents using scrbase, my suggestion would be for
scrbase to load iftex and then redefine any commands that it defined before for compatibility with existing use.

Sorry for lack of advance notice, it is hard to check every package in advance.

@mrpiggi

This comment has been minimized.

Copy link
Contributor Author

@mrpiggi mrpiggi commented Nov 4, 2019

There is really a very widespread convention (but sadly not universal) that commands starting \if.. are tex primitive \if with a matching \fi

Well, there are many fundamental packages e.g. etoolbox, which do not consider the widespread convention at all and use a syntax like \if...{test}{true-branch}{false-branch}. But I see your point, that it actually would make sense to distinguish between TeX primitives and LaTeX commands. But overwriting existing commands arbitrarily is not the best idea.

The collection of names in the new iftex is (apart for iftutex and ifluahbtex) taken from existing packages, and then some normalisation of mixed or lower case names.

I clearly see the good intentions but this causes an error with scrbase which provides the two command since 2002!

Sorry for lack of advance notice, it is hard to check every package in advance.

KOMA-Script is not a random package but a very basic bundle and used by some developers as well as many end-users. In my eyes it would not be a huge problem to check at least for newly defined commands/switches and raise a warning if they already exist instead of (re-)defining them ruthlessly.

That said it would be good not to break documents using scrbase, my suggestion would be for
scrbase to load iftex and then redefine any commands that it defined before for compatibility with existing use.

I thought, latex-dev was introduced to avoid such emergencies. I really appreciate how much effort the whole LaTeX team is doing. But the last release 2019-10-01 already generated a lot of workload for many developers. And after fixing many things raised by this release recently, Markus Kohm just stopped his work for an indefinite time.

@davidcarlisle

This comment has been minimized.

Copy link
Member

@davidcarlisle davidcarlisle commented Nov 4, 2019

KOMA-Script is not a random package but a very basic bundle and used by some developers as well as many end-users. In my eyes it would not be a huge problem to check at least for newly defined commands/switches and raise a warning if they already exist instead of (re-)defining them ruthlessly

Yes it is a major package but checking every combination of every package is hard, even contrib packages over which we have direct write access (eg the oberdiek collection) are not fully integrated into the automated test harness for the kernel.

The clash is inconvenient but as far as I can see adding

\RequirePackage{iftex}

to scrbase would make all existing documents work with the scrbase definitions taking priority and the iftex commands being defined otherwise,

@mrpiggi

This comment has been minimized.

Copy link
Contributor Author

@mrpiggi mrpiggi commented Nov 4, 2019

Thanks for the immediate reply. Let's see, if and when Markus will react.

@u-fischer

This comment has been minimized.

Copy link

@u-fischer u-fischer commented Nov 4, 2019

@mrpiggi which error do you get? Until now I see only a warning that scrbase detected the change.

@davidcarlisle

This comment has been minimized.

Copy link
Member

@davidcarlisle davidcarlisle commented Nov 4, 2019

@mrpiggi I saw an issue at https://komascript.de/node/2262 I would have left a comment (and an apology for the inconvenience) but it seems you need an account.

@mrpiggi

This comment has been minimized.

Copy link
Contributor Author

@mrpiggi mrpiggi commented Nov 4, 2019

@mrpiggi which error do you get? Until now I see only a warning that scrbase detected the change.

Well, just try with and without iftex

\documentclass{scrartcl}
%\usepackage{iftex}
\begin{document}
\ifpdftex{yepp}{nope}
\end{document}
@mrpiggi

This comment has been minimized.

Copy link
Contributor Author

@mrpiggi mrpiggi commented Nov 4, 2019

@mrpiggi I saw an issue at https://komascript.de/node/2262 I would have left a comment (and an apology for the inconvenience) but it seems you need an account.

...which currently can not be created anyways as Markus is "out of order" and deactivated the registration so far.

@u-fischer

This comment has been minimized.

Copy link

@u-fischer u-fischer commented Nov 4, 2019

@mrpiggi I know how to force the error, but did you see a concrete document failing on it? I mean the documentation says about ifpdftex

Diese Unterscheidung ist nur sehr selten wirklich von Nutzen

and I quite agree with this assessment.

@FrankMittelbach

This comment has been minimized.

Copy link
Contributor

@FrankMittelbach FrankMittelbach commented Nov 4, 2019

KOMA-Script is not a random package but a very basic bundle and used by some developers as well as many end-users. In my eyes it would not be a huge problem to check at least for newly defined commands/switches and raise a warning if they already exist instead of (re-)defining them ruthlessly.

It certainly isn't and in fact we do normally try to see check those kind of things (and most of the time succeed).

I guess part of the problem was that nobody thought that there could be a problem if you take 3 widely used packages by many other packages and combine them that any of this could happen.

I thought, latex-dev was introduced to avoid such emergencies. I really appreciate how much effort the whole LaTeX team is doing. But the last release 2019-10-01 already generated a lot of workload for many developers. And after fixing many things raised by this release recently, Markus Kohm just stopped his work for an indefinite time.

yes LaTeX-dev was for this, but hyperref is not part of the core (yet) nor are any other packages outside amsmath tools graphics and base and at this point in time it is not possible to extend latex-dev arbitrarily. So if I put out any "private" package I can't make a development version using latex-dev.

@mrpiggi

This comment has been minimized.

Copy link
Contributor Author

@mrpiggi mrpiggi commented Nov 4, 2019

First of all, I'm sorry if I sounded a bit rude.

yes LaTeX-dev was for this, but hyperref is not part of the core (yet) nor are any other packages outside amsmath tools graphics and base and at this point in time it is not possible to extend latex-dev arbitrarily. So if I put out any "private" package I can't make a development version using latex-dev.

OK, this sounds reasonable ;)

I guess part of the problem was that nobody thought that there could be a problem if you take 3 widely used packages by many other packages and combine them that any of this could happen.

Well, this only happened because previously not provided macros were additionally defined in order to cover different letter case variants.

@mrpiggi

This comment has been minimized.

Copy link
Contributor Author

@mrpiggi mrpiggi commented Nov 4, 2019

@mrpiggi I know how to force the error, but did you see a concrete document failing on it? I mean the documentation says about ifpdftex

Diese Unterscheidung ist nur sehr selten wirklich von Nutzen

and I quite agree with this assessment.

In order to run my documents with the three different engines (pdf|Lua|Xe)LaTeX, I use following snippet right after the document class

\ifpdftex{
  \usepackage[T1]{fontenc}
  \input glyphtounicode
  \pdfgentounicode=1
  \usepackage[ngerman=ngerman-x-latest]{hyphsubst}
}{
  \usepackage{fontspec}
}
@davidcarlisle

This comment has been minimized.

Copy link
Member

@davidcarlisle davidcarlisle commented Nov 4, 2019

Well, this only happened because previously not provided macros were additionally defined in order to cover different letter case variants.

@mrpiggi Yes, I didn't see how to make a coherent combined package without doing that, for example the
ifptex package defines its test in mixed case and lowercase \ifptex and \ifpTeX the iftex package defined mixed case names \ifXeTeX, the ifxetex and ifluatex defined lowercase names \ifxetex \ifluatex

So the only way of making something coherent and cover all the included packages was to define all the ifs in both forms. The one thing that all the packages had in common was that the commands were tex primitive if constructs.

While the existing setup didn't technically have a clash if you loaded scrbase and (say) ifluatex then \ifpdftex is a macro with arguments and \ifluatex is an if which is not really a documentable user interface.

That said, compatibility is pretty important here and between us we should avoid breaking existing documents.

As I say above my preferred solution would be for scrbase to load iftex then restore its definitions.
That restricts the legacy definitions to this combination, however if that is not possible for some reason then iftex could test for \ifpdftex being defined and if so avoid redefining it, I think this is not such a good solution as it works against the package aim of enforcing a standard set of definitions, but if it gives koma-script more time to adapt then this is a possibility.

With the koma-script issue tracker being locked I am not sure how best to proceed?

@davidcarlisle

This comment has been minimized.

Copy link
Member

@davidcarlisle davidcarlisle commented Nov 4, 2019

specifically we could add

% special compat for scrbase
\ifx\scr@ifpdftex\@undefined\else
 \let\ifpdftex\scr@ifpdftex
\fi
\ifx\scr@ifVTeX\@undefined\else
 \let\ifVTeX\scr@ifVTeX
\fi

at the end of iftex.sty

or at least we could do something along those lines (not that exactly as it would break \RequirePDFTeX (which would need to be defined in terms of the mixed case \ifPDFTEX)

@mrpiggi

This comment has been minimized.

Copy link
Contributor Author

@mrpiggi mrpiggi commented Nov 4, 2019

@davidcarlisle Thank you very much for your efforts. That would be a very pragmatic solution until Markus reappears ;)

davidcarlisle added a commit that referenced this issue Nov 4, 2019
@davidcarlisle

This comment has been minimized.

Copy link
Member

@davidcarlisle davidcarlisle commented Nov 4, 2019

@mrpiggi can you try the version I just checked in with some real documents, I could push to ctan tomorrow if it looks OK

@mrpiggi

This comment has been minimized.

Copy link
Contributor Author

@mrpiggi mrpiggi commented Nov 5, 2019

With the current version and without using scrbase

\documentclass{article}
\usepackage{iftex}
\begin{document}
test
\end{document}

I get

! Incomplete \ifx; all text was ignored

twice. This is probably because \ifpdftex is read as \iftrue within the \ifx-check, isn't it? Calling

\ifx\scr@ifpdftex\@undefined\else
 \expandafter\let\csname ifpdftex\endcsname\scr@ifpdftex
 \IFTEX@protected\def\RequirePDFTeX{\IFTEX@Require\ifpdftex{pdfTeX}\fi}
\fi
\ifx\scr@ifVTeX\@undefined\else
 \expandafter\let\csname ifVTeX\endcsname\scr@ifVTeX
\fi

resolves the problem. This would be fine for me.

Package scrbase actually allows to use the option internalonly in order to not define \ifpdftex and \ifVTeX but only the internal macros (\scr@...). In order to set up compatibility in a clean manner, the checks would actually have to be done before the definitions of iftex:

\ifx\scr@ifpdftex\@undefined\else
  \ifx\scr@ifpdftex\ifpdftex
    %restore \ifpdftex afterwards
  \fi
\fi
\ifx\scr@ifVTeX\@undefined\else
  \ifx\scr@ifVTeX\ifVTeX
    %restore \ifVTeX afterwards
  \fi
\fi

and the compatibility settings should maybe be done at the end, if checks were positive. But this is not that urgent in my eyes as not a single package or class uses this option currently as far as I could see.

@davidcarlisle

This comment has been minimized.

Copy link
Member

@davidcarlisle davidcarlisle commented Nov 5, 2019

Sorry about the obvious failure in the checkin last night, I was obviously too tired.
Before seeing your second comment it had occurred to me that the test wasn't good as if scrbase changes it would not be right I think that actually you just need

\ifx\scr@ifpdftex\ifpdftex

without testing if scr@ifpdf is defined as given that \ifpdftex is definitely defined to something by this point, that will only be true if scr@ifpdftex is defined and \ifpdftex is equal to it.

@mrpiggi

This comment has been minimized.

Copy link
Contributor Author

@mrpiggi mrpiggi commented Nov 5, 2019

Sorry about the obvious failure in the checkin last night, I was obviously too tired.

This sounds familiar to me ;)

Before seeing your second comment it had occurred to me that the test wasn't good as if scrbase changes it would not be right I think that actually you just need

\ifx\scr@ifpdftex\ifpdftex

without testing if scr@ifpdf is defined as given that \ifpdftex is definitely defined to something by this point, that will only be true if scr@ifpdftex is defined and \ifpdftex is equal to it.

I'm not sure if you got my point. With option internalonly package scrbase does not define \ifpdftex but with the current package iftex it is defined in the scrbase manner even it was not defined before. See this example with and without iftex

\documentclass{article}
\usepackage[
internalonly
]{scrbase}
%\usepackage{iftex}
\begin{document}
\makeatletter

\meaning\ifpdftex

\meaning\scr@ifpdftex
\end{document}

But as I said, this is not an urgent issue and could maybe resolved, when Markus is back.

@davidcarlisle

This comment has been minimized.

Copy link
Member

@davidcarlisle davidcarlisle commented Nov 5, 2019

@mrpiggi I added some test files with and without the overload option, I think it does the right thing in all cases now, could you test again, thanks.

@mrpiggi

This comment has been minimized.

Copy link
Contributor Author

@mrpiggi mrpiggi commented Nov 5, 2019

@davidcarlisle Thanks a lot, everything works as expected.

@davidcarlisle

This comment has been minimized.

Copy link
Member

@davidcarlisle davidcarlisle commented Nov 5, 2019

pushed to ctan, it should show up distributions in a few days.

mrpiggi added a commit to tud-cd/tudscr that referenced this issue Nov 6, 2019
@davidcarlisle

This comment has been minimized.

Copy link
Member

@davidcarlisle davidcarlisle commented Nov 7, 2019

@mrpiggi It seems you have write access to the koma-script discussion, I read
https://komascript.de/node/2262#comments
(via google translate)

If you could note, that I can respond to bugs and change requests that are posted to the package bug tracker, but I can't (normally) respond to complaints about the package on a site I don't follow in a language I can't read).

If Markus (or anyone acting on his behalf) made a bug report here we could look although no promises, I think compatibility is pretty good considering the starting position, but we would look at anything reported)

@mrpiggi

This comment has been minimized.

Copy link
Contributor Author

@mrpiggi mrpiggi commented Nov 7, 2019

The essence of the discussion there is that the definition of \ifpdftex and \ifVTeX causes compatibility issues with existing documents that use these commands as the current workaround, since the current workaround only works if iftex is loaded after scrbase. Swapping the load order as in

\RequirePackage{iftex}
\RequirePackage{scrbase}
\documentclass{article} 
\begin{document}
\meaning\ifpdftex
 
\meaning\ifVTeX
\end{document}

still raises the same issue.

Markus's point is still that the definition of the two commands is simply not necessary and rather highly problematic.

@u-fischer

This comment has been minimized.

Copy link

@u-fischer u-fischer commented Nov 7, 2019

The essence of the discussion there is that the definition of \ifpdftex and \ifVTeX causes compatibility issues with existing documents that use these commands as the current workaround, since the current workaround only works if iftex is loaded after scrbase.

yes, but scrbase would have this problem with every package or class defining a different \ifpdftex as it doesn't fight for its definition. Try e.g. to load pdfcprot before scrbase.

Markus's point is still that the definition of the two commands is simply not necessary and rather highly problematic.

Well having an \ifluatex .. \fi and \ifpdftex{true}{false} side by side isn't really a good situation. And having an generic engine test in a package which is more or less only used together with some specific classes neither. Even more as scrbase has only user tests for pdftex and VTeX, but ignores the other engines.

@davidcarlisle

This comment has been minimized.

Copy link
Member

@davidcarlisle davidcarlisle commented Nov 7, 2019

@mrpiggi well yes but that's a design decision in scrbase that it lets an existing definition through, it could choose not to to that if you want those meaning to show the scrbase definitions. That is not a choice that I control.

As you see with the original comment I'm open to be persuaded but I think the argument for not defining them at all in iftex is a lot weaker. scrbase could choose to overrule here.

To be honest it's clear looking at the scrbase code that it was known that these were bad names, hence the specific option not to use those names and some slightly different clash needing a workaround with pdfcprot.

Given the number of contributed packages the occasional clash is unfortunately necessary although we do go to some lengths to avoid that (or to pre-warn people if we can't avoid it and notice in time). It's a shame that we missed the clash with scrbase initially but it happened. I restructured the code to avoid the problem as reported, as you know.

I'm sorry if Markus still thinks the change isn't enough but to be honest it's hard to have a rational discussion about the best way forward if scrbase discussion forum is locked out for new users and Markus doesn't communicate and adds moral pressure that he will give up and it's all our fault.

I'll leave this closed but again, if you (or anyone) wants to open a new issue with a plausible case where this causes problems in a real document and not just where the package loading order has been deliberately chosen to break, I'm open to be persuaded.

@mrpiggi

This comment has been minimized.

Copy link
Contributor Author

@mrpiggi mrpiggi commented Nov 7, 2019

First of all, I would like to make it very clear that I am completely satisfied with the solution found. In my opinion, it was long overdue to bundle all the packages. For me personally, the thing is not particularly tragic, because I used \ifpdftex only for the selection of fontspec or fontenc so far. Since there is finally a switch \iftutex, I will adapt my documents consecutively.

I just wanted to provide the feedback from Markus here again, because @davidcarlisle has commented on the discussion on komascript.de. I also made it quite clear that @davidcarlisle was very quick and constructive in solving the problem.

Since Markus read along there, I strongly assume that he also knows this issue here. Therefore, I think it is unnecessary to spend more time on this problem as long as he does not answer. I'll refer to this issue again right now. Nevertheless, if the discussion there evolves, I would at least summarize the core contents here, as I intended with my last comment.

@davidcarlisle

This comment has been minimized.

Copy link
Member

@davidcarlisle davidcarlisle commented Nov 7, 2019

@mrpiggi thanks for being the go-between:-)

@mrpiggi

This comment has been minimized.

Copy link
Contributor Author

@mrpiggi mrpiggi commented Nov 7, 2019

There is really a very widespread convention (but sadly not universal) that commands starting \if.. are tex primitive \if with a matching \fi

As a completeness, how do you think an if-branch should be named, which is defined in the LaTeX way? Probably in the UpperCamelCase notation, as it is done by the xparse package, right? (\IfTestSomething{testsubject}{true-branch}{false-branch})

@davidcarlisle

This comment has been minimized.

Copy link
Member

@davidcarlisle davidcarlisle commented Nov 7, 2019

@mrpiggi yes perhaps. That would also match the format's \IfFileExists, \IfTargetDateBefore or for package level use \@if as in \@ifdefinable, \@ifundefined, latex naming conventions are a bit more variable than would be nice but as \if...\fi matching is so strongly built into tex's parser and cuts across grouping and other constructs it is really useful when reading code if you can visually see the \if tokens. But I know other major packages like etoolbox define macros with arguments that start with \if.. so this is just a background feeling that probably influences thinking about the issue, rather than a real convention that is being broken.

@u-fischer

This comment has been minimized.

Copy link

@u-fischer u-fischer commented Nov 7, 2019

There is really a very widespread convention (but sadly not universal) that commands starting \if.. are tex primitive \if with a matching \fi

As a completeness, how do you think an if-branch should be named, which is defined in the LaTeX way? Probably in the UpperCamelCase notation, as it is done by the xparse package, right? (\IfTestSomething{testsubject}{true-branch}{false-branch})

in the tagpdf package I decided to use the end marker "TF" to mark booleans and to put the if somewhere in the middle: tagmcifinTF. This reflect the internal expl3 syntax: \tl_if_blank:TF only without the underscores and colons.

@norbusan

This comment has been minimized.

Copy link

@norbusan norbusan commented Nov 8, 2019

After reading the thread here and MK's comments in the forum, I am more and more inclined to not do any workarounds. His language is extremely rude, uncooperative, and completely besides the point. With his attitude, no changes to any interface can be introduced, because he would have to make changes. As much as I honor his contribution, treating him like the "Prinzessin auf der Erbse" is something that I don't support. He should, like everyone else, remain considerate and collaborate.

@FrankMittelbach

This comment has been minimized.

Copy link
Contributor

@FrankMittelbach FrankMittelbach commented Nov 8, 2019

As a completeness, how do you think an if-branch should be named, which is defined in the LaTeX way? Probably in the UpperCamelCase notation, as it is done by the xparse package, right? (\IfTestSomething{testsubject}{true-branch}{false-branch})

xparse uses \IfValueTF etc for true/false which is a variant of what expl3 internally does (to denote the branches with T and F) allowing for variants like \IfValueF and I think this is a rather readable notation. As far as the first part of package command names is concerned, I would say the final vote is still out, though I personally favor the CamelCase approach for those.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.