Make cupstestppd legacy by metabiswadeep · Pull Request #6 · OpenPrinting/libppd

metabiswadeep · 2023-01-21T19:38:17Z

Migrated cupstestppd.c from cups to ppd-test.c in libppd to make it legacy.

tillkamppeter · 2023-01-21T19:43:45Z

ppd/ppd-test.c

+ * Include necessary headers...
+ */
+
+#include <cups/cups-private.h>


Do not use any "private" interfaces from CUPS, they are no stable APIs and only valid for code inside the CUPS of the version you have taken the cupstestppd.c from. Please remove this line and do not use any functions whose name starts with _cups. Please check these functions and check whether libppd (or libcupsfilters) contains them (or at least similar ones). I have overtaken several of them into libppd (but then renamed them to start with ppd or _ppd).

_cupsSetLocale, _cupsLangPuts and _cupsLangPrintf is in langprintf.c in cups, an alternative of which is not present in libppd. Should I migrate langprintf.c to libppd as well?

These functions are to set the user's UI language and to output the translations of given strings into the selected UI language. This way cupstestppd outputs its report in the user language if CUPS has appropriate translations.

In a first approach one could remove the _cupsSetLocale() call and replace the other functions by fputs() and fprintf() and the report will always be in English (the strings in the source code which are supplied to the functions).

Actually you should replace the calls by calls of the log function (which you supply to ppdTest() in its parameter list). You will also not have translations with that, but by simply replacing the log function with a more sophisticated one one could easily add translation support later.

But generally, I think translations are not that important here. PPD files are deprecated and will go away in the future, especially when the New Architecture is finally in place. So we should no put too much work into libppd and also, the former cupstestppd, provided via your port into libppd will mainly be used by retro-fitting Printer Applications to check the integrity of PPD files or by developers/maintainers of such Printer Applications to handle bug reports and support requests. Developers and maintainers usually understand English and translated logs are even counter-productive for them.

tillkamppeter · 2023-01-21T19:45:59Z

ppd/ppd-test.c

+
+#include <cups/cups-private.h>
+#include <cups/dir.h>
+#include <cups/ppd-private.h>


Also remove this line, and check where functions with names starting with _ppd are used. check through libppd whether I have overtaken them. Using "private" functions of libppd is no problem, by the way, as you are in libppd here.

…into cupstestppd

tillkamppeter · 2023-02-06T12:39:35Z

ppd/ppd.h

-                  int q_with_v, int v_with_q, int root_present, int files,
-                  cups_array_t *file_array, cups_array_t *stdin_array);
+
+typedef struct ignore_parameters_s


Please use real bit fields here, not structs. bitfields are integer variables where each bit represents one boolean state and the bits are accessed via bit-wise AND and OR operations. Use the bitfields which the ppdTest() function is suing internally also as parameter for it and let main() of testppdfile.c do the |= to add the bits according to the user-supplied command line arguments.

tillkamppeter · 2023-02-08T17:43:30Z

ppd/testppdfile.c

+  WARN_ALL = 255
+};
+
+


Please move these enums into ppd/ppd.h, as everyone who wants to call ppdTest() needs them. Then ppdTest() itself can use them, testppfile.ccan use them and everyone who does #include <ppd/ppd.h> for using ppdTest() ...

tillkamppeter · 2023-02-08T17:59:25Z

Otherwise it looks all OK now for me. Did you also check that everything uses either public APIs or any public or private API of libppd? No use of private APIs oCUPS and also PPD-related functions onl;y of libppd, not of CUPS. Also no (f)printf() or similar in library functions (logging via log function there).

metabiswadeep · 2023-02-08T20:19:05Z

Otherwise it looks all OK now for me. Did you also check that everything uses either public APIs or any public or private API of libppd? No use of private APIs oCUPS and also PPD-related functions onl;y of libppd, not of CUPS. Also no (f)printf() or similar in library functions (logging via log function there).

According to what I have checked, there is not any function of "-private.h" of cups used in it. Also logging fuction has been used for everything and fprintf() or fputs() has not been used in ppd-test.c. However, puts has been used in testppdfile.h to get the user readable output.

tillkamppeter · 2023-02-08T23:44:52Z

ppd/ppd.h

+			int files, cups_array_t *file_array);
+
+//Error warning overrides...
+typedef enum overrides_e


Could you move this type definition to the type definition section (enums) of ppd/ppd.h.
Could you also change the names to start with PPD_TEST_ or ppd_test_.

tillkamppeter · 2023-02-08T23:49:52Z

ppd/testppdfile.c

+
+    if (argv[i][0] == '-')
+    {
+      _ppdArrayAddStrings(file_array,"");


For these simple additions you could use cupsArrayNew() and cupsArrayAdd() instead off the functions of libppd's private API, to keep the private API inside libppd. Then you could also remove #include <ppd/array-private.h> at the top of the file.

tillkamppeter · 2023-02-08T23:54:30Z

Could you also change all the comments from /* ... */ to // ... as I have done it this way in the rest of libppd (actually following the coding style of PAPPL and not of CUPS).

tillkamppeter · 2023-02-09T09:54:35Z

Thanks for the changes, but there are some issues:

When using // ... for comments always have a space between // and the comment.
The change from _ppdArrayAddStrings() to cupsArrayAdd() was only needed in testppdfile.c NOT in ppdTest(). The latter is inside libppd and therefore can use libppd's private interfaces. Could you change this back?
The values of the enum also need to get renamed: WARN_ALL -> PPD_TEST_WARN_ALL ...

Please also compile the code and test some PPDs with the new utility to check whether you get the same results as with the original utility from CUPS.

tillkamppeter · 2023-02-09T10:47:38Z

ppd/ppd-test.c

-  struct lconv *loc;       // Locale data 
+  int i, j, k, m, n;       //  Looping vars 
+  size_t len;               //  Length of option name 
+  const char *ptr;       //  Pointer into string 


Could you run a "// " -> "// " over the code?

ppd/ppd-test.c

tillkamppeter · 2023-02-09T10:51:24Z

ppd/ppd-test.c

@@ -82,48 +83,48 @@ static int valid_path(const char *keyword, const char *path, int errors,
 static int valid_utf8(const char *s);


-//'ppdTest()' - Test the correctness of PPD files.
+// 'ppdTest()' - Test the correctness of PPD files.


 cups_array_t *ppdTest(int ignore, int warn, char *rootdir,


Could you have one line for each parameter and add a short comment for each parameter what it is for? See the other functions in libppd. Also these comments are used for auto-generation of documentation.

metabiswadeep · 2023-02-09T11:54:18Z

Thanks for the changes, but there are some issues:

When using // ... for comments always have a space between // and the comment.

The change from _ppdArrayAddStrings() to cupsArrayAdd() was only needed in testppdfile.c NOT in ppdTest(). The latter is inside libppd and therefore can use libppd's private interfaces. Could you change this back?

The values of the enum also need to get renamed: WARN_ALL -> PPD_TEST_WARN_ALL ...

Please also compile the code and test some PPDs with the new utility to check whether you get the same results as with the original utility from CUPS.

Is it not possible to use cupsArrayAdd in ppdTest as I believe it will serve the purpose better as compared to _ppdArrayAddStrings?

tillkamppeter · 2023-02-09T12:16:12Z

What we want is a CUPS array where each element contains one line of the output text. _ppdArrayAddStrings() allows supplying multi-line strings and they get separated into the individual lines. Duplicate lines are dropped. cupsArrayAdd() just adds the strings, if each array entry should be a line of the output, you need to take care that each string you add is actually a single line (no \n). Also, duplicate lines will simply get added.

tillkamppeter · 2023-02-14T21:38:53Z

ppd/testppdfile.c


  (cupsArrayFirst(output));
-  for (int j = 1; j< len_output; j++)
+  for (int j = 0; j< len_output-1; j++)


Also wrong. It must be

for (int j = 0; j < len_output; j++)

The len_output includes the initialization value of " " for the array, which we don't want to be printed in the output screen, so I am using len_output-1 times looping so that " " is not printed in output screen.

You do not need to add a " " line to the array to initialize it. The array is already initialized and accepts regular lines right after creating it with cupsArrayNew3(), when it is still empty. So do not add this empty line at the furst place.

Now, I have removed the " " line. However, j will loop till len_output-1 as I am using cupsArrayNext as well within the loop, so in I use len_output instead, the length of array will be exceeded when calling cupsArrayNext and will give errors.

For outputting all element of a CUPS array I even do not define an integer variable to count the element. I simply do

char *line; for (line = (char *)cupsArrayFirst(output); line; line = (char *)cupsArrayNext(output)) puts(line);

This outputs all the elements of the array, without needing to get the number of elements and without treating the first element differently. You find this way to treat a CUPS array at many places in CUPS, libcupsfilters, and other OpenPrinting projects.

tillkamppeter · 2023-02-15T11:10:49Z

ppd/testppdfile.c

  int files;			   // Number of files 
  cups_array_t	*output;  // Output array
-  int len_output;   // Length of the output array
+  char *line;   // Length of the output array


This is not the length of the output array any more ...

tillkamppeter · 2023-02-15T11:46:06Z

A suggestion to improve the architecture of the ppdTest() function:

Make the creation of a human-readable report array optional. For that add to the parameter list (as last item) a parameter named report of type cupsArray *. If NULL is supplied no report array is created, if a pointer which is set to NULL is supplied, a CUPS array is created with cupsArrayNew3() and the pointer is set to pinting to that array, if a non-Null pointer is supplied, it is assumed to be an array of the right type and new lines aget added to it (allows to put several reports, one after the other into one array).
The return value will be of type interger. 1 means that the supplied PPD has passed the test and 0 means that it has failed. -1 is an error, like PPD does not exust, out of memory, ...
At the point where you create the output array, do

    if (report && *report == NULL)
    {
      *report = cupaArrayNew3(...);
      if (*report == NULL)
      {
        ...
        return (-1);
      ]
    ]

At the point where you want to output a result of some check do (especially this way you also do not have to put in the string constant for the message twice, do not use the prefix "ppdTest: " in the string constant in the first line):

    sprintf(str_format, "...", XXX, YYY);
    if (*report)
      cupsArrayAdd(*report, (void *)str_format);
    if (log) log(ld, CF_LOGLEVEL_..., "ppdTest: %s", str_format);

tillkamppeter · 2023-02-15T12:15:32Z

I have edited the code I have suggested in the previous comment, concerning the "ppdTest: " prefix.

Also with this it is easy for a developer to use the ppdTest() function as they prefer: Either to simply check the validity of the PPD without any reporting, reporting in the caller's log (by supplying a log function), reporting into the array, for stdout output or for parsing the report for futher analysis ...

metabiswadeep · 2023-02-17T18:43:44Z

A suggestion to improve the architecture of the ppdTest() function:

Make the creation of a human-readable report array optional. For that add to the parameter list (as last item) a parameter named report of type cupsArray *. If NULL is supplied no report array is created, if a pointer which is set to NULL is supplied, a CUPS array is created with cupsArrayNew3() and the pointer is set to pinting to that array, if a non-Null pointer is supplied, it is assumed to be an array of the right type and new lines aget added to it (allows to put several reports, one after the other into one array).

Instead of making a new report variable can't the output variable be modified to perform this functionality?

tillkamppeter · 2023-02-17T23:57:07Z

I want to make the return value of ppdTest() an integer, to easily get the result, PASS, FAIL, or error. Therefore I want to have the pointer to the report in the parameter list of ppdTest(). This also gives flexibility. You can have an existing array, for example containing a previous report, or simply an empty array which you created. Then you simply supply a pointer to this array and ppdTest() adds its report to the end of this array. You can have a variable which should hold the report. You set it to NULL and supply a pointer to this variable. The ppdTest() creates the array for you. Or you supply NULL if you do not want to have a report, if the integer result and perhaps also the logging via log function is good enough for your. So this should server everyone ...

metabiswadeep · 2023-02-18T04:46:47Z

I want to make the return value of ppdTest() an integer, to easily get the result, PASS, FAIL, or error. Therefore I want to have the pointer to the report in the parameter list of ppdTest(). This also gives flexibility. You can have an existing array, for example containing a previous report, or simply an empty array which you created. Then you simply supply a pointer to this array and ppdTest() adds its report to the end of this array. You can have a variable which should hold the report. You set it to NULL and supply a pointer to this variable. The ppdTest() creates the array for you. Or you supply NULL if you do not want to have a report, if the integer result and perhaps also the logging via log function is good enough for your. So this should server everyone ...

If PPD-test does not return the output array( for this matter the report array), then how can it be accessed by the testppdfile to provide the human readable output if the user wants the report to be shown?

tillkamppeter · 2023-02-18T08:46:36Z

I hope you know the difference between call-by-value and call-by-reference. For getting the report back I put cups_array_t **report and not cups_array_t *report into the parameter list of ppdTest(). So I pass the address of a cups_array_t. So ppdTest() uses, modifies, or creates the report array on this address. Afterwards the caller reads the array from this address.

tillkamppeter · 2023-02-18T08:53:33Z

You call ppdTest() as follows then:

cups_array_t *report = NULL;
int result;

...
result = ppdTest(..., &report);
...
if (report)
{
  char *line;
  for (line = (char *)cupsArrayFirst(output);
       line;
       line = (char *)cupsArrayNext(output))
    puts(line);
}
...

tillkamppeter · 2023-02-19T00:24:18Z

Looks great! Thank you. Did you compile and test it?

Will have another look and if all is fine I will merge it.

metabiswadeep · 2023-02-19T04:22:18Z

Looks great! Thank you. Did you compile and test it?

Yes, I did compile and test it.

tillkamppeter · 2023-02-19T08:16:23Z

OK. Could you do the following corrections:

Remove the file testppdfile from the repository. It is a temporary, machine-generated script from libtool. The GIT repo should only contain human-generated files.
Avoid ANY duplication of string constants. If you do not sprintf() the output string from a format string and variables, but simply have a constant output string, simply do:

    str_format = "      **FAIL**  REQUIRED DefaultPaperDimension\n"
                 "                REF: Page 103, section 5.15.");
    if (*report)
      cupsArrayAdd(*report, (void *)str_format);
    if (log) log(ld, CF_LOGLEVEL_ERROR, "ppdTest: %s", str_format);

DO NOT use sprintf() but use snprintf(). Set as the length the length of the buffer - 1, but do not put the number, put sizeof(str_format) - 1, so that you can simply redefine the size of str_format whenever you want, without needing to correct hundreds of lines.
If you found a failing condition in the PPD and report FAIL, do not use the log level CF_LOGLEVEL_ERROR but also CF_LOGLEVEL_DEBUG. Use CF_LOGLEVEL_ERROR only if there is an error with reading the PPD file or another error of ppdTest(). A failing test result is not considered an error here.
Check the correct indentation for the declaration of ppdTest() in ppd/ppd.h.

tillkamppeter · 2023-02-19T20:48:25Z

Now it looks all correct, but could you have another look through the indentation? It is 2 spaces per level.

The tab width is 8 spaces (tab character fills up to the next 8-char grid line).

See DEVELOPING.md.

tillkamppeter · 2023-02-21T09:13:49Z

Your code produces tons of warnings. Could you fix them ASAP (with a new PR)?

metabiswadeep · 2023-02-21T18:26:55Z

Your code produces tons of warnings. Could you fix them ASAP (with a new PR)?

Sure I'll do it.

metabiswadeep · 2023-02-21T18:35:02Z

Your code produces tons of warnings. Could you fix them ASAP (with a new PR)?

I see that the warnings have been taken care of by you. Sorry that I couldn't look into it. I had an exam today so didn't get the time to do it.

Migrate cupstestppd

1f6aa3b

tillkamppeter reviewed Jan 21, 2023

View reviewed changes

metabiswadeep added 6 commits February 1, 2023 21:22

Merge branch 'OpenPrinting:master' into cupstestppd

0201e0f

Changed functions

de3ed63

Merge branch 'cupstestppd' of https://github.com/metabiswadeep/libppd …

5c52423

…into cupstestppd

Update ppd-test.c

d43ba5e

Human readable output

b7548ac

Some indentation and bitsfield

332d329

tillkamppeter reviewed Feb 6, 2023

View reviewed changes

metabiswadeep added 3 commits February 8, 2023 03:59

Changed bitsfield and some more indentation

04e71c4

Indentation

08a671c

Indentation completed

717888a

tillkamppeter reviewed Feb 8, 2023

View reviewed changes

metabiswadeep changed the title ~~Migrate cupstestppd~~ Make cupstestppd legacy Feb 8, 2023

Move enums to ppd.h

f3ca378

tillkamppeter reviewed Feb 8, 2023

View reviewed changes

metabiswadeep added 2 commits February 9, 2023 10:33

Made necessary modifications

25166a4

Changed enum definition names

a145078

Modified files

e6656bf

tillkamppeter reviewed Feb 9, 2023

View reviewed changes

ppd/ppd-test.c Show resolved Hide resolved

tillkamppeter reviewed Feb 9, 2023

View reviewed changes

Requested changes

9ca9841

tillkamppeter reviewed Feb 14, 2023

View reviewed changes

metabiswadeep added 2 commits February 15, 2023 14:03

Added null check

82524d9

Changed loop

d145859

tillkamppeter reviewed Feb 15, 2023

View reviewed changes

Update testppdfile.c

2d8854a

metabiswadeep added 2 commits February 19, 2023 05:08

Added report

20024ec

Removed bogus var

59b6d5a

metabiswadeep added 3 commits February 19, 2023 23:06

Made necessary changes

aa31083

Indentation

2ac2097

Delete testppdfile

682d47a

metabiswadeep added 4 commits February 20, 2023 15:47

Indentation

8dcdc81

Some more indentation

427efa0

Correcting indentation

63fec0d

Update ppd-test.c

f06b63a

tillkamppeter merged commit 7775298 into OpenPrinting:master Feb 20, 2023

Conversation

metabiswadeep commented Jan 21, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

metabiswadeep Jan 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tillkamppeter commented Feb 8, 2023

Uh oh!

metabiswadeep commented Feb 8, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tillkamppeter commented Feb 8, 2023

Uh oh!

tillkamppeter commented Feb 9, 2023

Uh oh!

tillkamppeter Feb 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

metabiswadeep commented Feb 9, 2023

Uh oh!

tillkamppeter commented Feb 9, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tillkamppeter Feb 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tillkamppeter commented Feb 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tillkamppeter commented Feb 15, 2023

Uh oh!

metabiswadeep commented Feb 17, 2023

Uh oh!

tillkamppeter commented Feb 17, 2023

Uh oh!

metabiswadeep commented Feb 18, 2023

Uh oh!

tillkamppeter commented Feb 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tillkamppeter commented Feb 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tillkamppeter commented Feb 19, 2023

Uh oh!

metabiswadeep commented Feb 19, 2023

Uh oh!

tillkamppeter commented Feb 19, 2023

Uh oh!

tillkamppeter commented Feb 19, 2023

Uh oh!

tillkamppeter commented Feb 21, 2023

metabiswadeep Jan 21, 2023 •

edited

Loading

tillkamppeter Feb 9, 2023 •

edited

Loading

tillkamppeter Feb 15, 2023 •

edited

Loading

tillkamppeter commented Feb 15, 2023 •

edited

Loading

tillkamppeter commented Feb 18, 2023 •

edited

Loading

tillkamppeter commented Feb 18, 2023 •

edited

Loading