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

metanorma site generate ignore errors collected on compile #301

Open
2 tasks
CAMOBAP opened this issue Mar 9, 2023 · 17 comments
Open
2 tasks

metanorma site generate ignore errors collected on compile #301

CAMOBAP opened this issue Mar 9, 2023 · 17 comments
Assignees
Labels
enhancement New feature or request

Comments

@CAMOBAP
Copy link
Contributor

CAMOBAP commented Mar 9, 2023

Intro

During investigation of mn2pdf fatal error I have noted that metanorma finishes successfully in case of Fatal: ... errors

Problem

Compiler::compile_file returns an array of fatal errors which is ignored:

select_source_files.each { |source| compile(source) }

Proposed solution

  • exit code must not be zero if we faced a fatal error
  • print fatal error by the end of site generation

@opoudjis @ronaldtse I would like to get your feedback on proposed solution

@opoudjis
Copy link
Contributor

I'm afraid metanorma-cli falls in between areas of responsibility: I didn't author it, and I haven't particularly been maintaining it. I agree that fatals should be propagated up; wouldn't the simplest thing to do be to raise an error if a fatal error is returned?

(I suppress errors in metanorma gems so that the file can be processed to completion, with a complete list of errors, and a complete *.xml.abort file generated for debugging. But nothing prevents an error being re-raised by whatever is consuming the list of errors.)

I am vague as to whether the @fatalerror variable in metanorma-standoc is what is going into the compile.error in CLI. I don't think they are: the errors in metanorma gem lib/metanorma/compile.rb seem to be populated only from isodoc crashes. So when process_input_adoc() in that file processes the Asciidoctor (as inherited from metanorma-standoc). There is an attempt to do so in lib/metanorma/input/asciidoc.rb in asciidoctor_validate(), grabbing content from STDERR so long as it has the right prefix; but I'm pretty sure it's ignoring @fatalerror.

(OTOH, if fatalerror is triggered in metanorma-standoc, then no Semantic XML file will be generated, and that alone should be enough to abort execution, because the isodoc conversion has nothing to proceed from.)

I'm afraid I do not and will not have the free time to fix this myself, but that at least gives you some pointers. If errors can be trapped in isodoc and standoc, and then propagated back up through well-managed channels to metanorma gem and metanorma-cli, that is a good thing, but as you can see, that propagation until now has been piecemeal and haphazard.

@CAMOBAP
Copy link
Contributor Author

CAMOBAP commented Mar 23, 2023

I'm afraid metanorma-cli falls in between areas of responsibility: I didn't author it, and I haven't particularly been maintaining it. I agree that fatals should be propagated up; wouldn't the simplest thing to do be to raise an error if a fatal error is returned?

I think we already have a strict node for this

But the original problem is that metanorma returns zero exit code in that case which is definitely wrong

To be on the same page I don't propose any massive refactoring/changes, just two simple things:

  • non-zero exit code
  • print fatal error to the stdout

@opoudjis
Copy link
Contributor

opoudjis commented May 4, 2023

FWIW, I'm still in agreement with this; should I assign you the task, @CAMOBAP ?

@ronaldtse ronaldtse added the enhancement New feature or request label Jun 23, 2023
@ronaldtse ronaldtse moved this to 🌋 Urgent in Metanorma Jun 23, 2023
@ronaldtse
Copy link
Contributor

@CAMOBAP how's the progress of this task? Thanks.

@ronaldtse
Copy link
Contributor

ronaldtse commented Feb 9, 2024

@CAMOBAP is working on this. Thanks!

@opoudjis
Copy link
Contributor

@CAMOBAP Be aware, if you aren't already, that I have rationalised logging during the Asciidoctor parsing component of compilation, so that the @log object contains objects for all warnings, which are then written to disk; if any of those warnings have a severity level of zero, execution is aborted. There is no @fatalerrors object any more, storing such errors separately.

Ideally any processing of errors should be integrated into that log, but I don't know how straightforward that would be.

@CAMOBAP
Copy link
Contributor Author

CAMOBAP commented Feb 13, 2024

The #320 PR is based on errors from https://github.com/metanorma/metanorma/blob/main/lib/metanorma/compile.rb#L197-L199 can I rely on this logic?

Ideally any processing of errors should be integrated into that log

got it maybe you can suggest modules/files to look at?

opoudjis added a commit that referenced this issue Feb 19, 2024
@opoudjis
Copy link
Contributor

The #320 PR is based on errors from https://github.com/metanorma/metanorma/blob/main/lib/metanorma/compile.rb#L197-L199 can I rely on this logic?

For now. The problem is that ALL logs need to output to the same spot, and at the moment, only standoc (and children) are doing so. I am about to have relaton pass its log errors back to standoc, so standoc can add them to its log. But it seems to me now that the standoc log, in turn, should not be output by standoc, but by metanorma gem.

Ideally any processing of errors should be integrated into that log

got it maybe you can suggest modules/files to look at?

yes:

The logging class is https://github.com/metanorma/metanorma-utils/blob/main/lib/utils/log.rb

@log is a class variable in metanorma-standoc. I presume it needs to turn into a class variable initialised in metanorma, and passed to metanorma-standoc and then isodoc to populate, before it is output to disk at the end of metanorma processing.

@log it turns out is already being passed from standoc to mn-requirements: https://github.com/metanorma/metanorma-standoc/blob/a16d7b909329842bf3ad32793349908c80ed4e56/lib/metanorma/standoc/reqt.rb#L38

https://github.com/metanorma/metanorma-standoc/blob/a16d7b909329842bf3ad32793349908c80ed4e56/lib/metanorma/standoc/base.rb#L43 is where @log is initialised :

@log = Metanorma::Utils::Log.new

See how @log is used in standoc. In particular:

  • Log a fatal error: @log.add("Fatal Error", nil, e.message, severity: 0)
  • Log a warning: @log.add("Bibliography", clause, "Reference #{id} is missing a title", severity: 1)
  • Output the log file to disk: @log.write("#{@output_dir}#{@filename}.err.html")

So instead of this set up, we would pass the @log to standoc on invoking standoc in compile.rb (and create @log inside of standoc only if @log is not passed in):

https://github.com/metanorma/metanorma/blob/main/lib/metanorma/processor.rb#L20

def input_to_isodoc(file, filename, options = {})
      Metanorma::Input::Asciidoc.new.process(file, filename,
                                             @asciidoctor_backend, options)
    end

https://github.com/metanorma/metanorma/blob/main/lib/metanorma/input/asciidoc.rb

def process(file, filename, type, options = {})
        require "asciidoctor"
        out_opts = {
          to_file: false, safe: :safe, backend: type, header_footer: true,
          attributes: ["nodoc", "stem", "docfile=#{filename}",
                       "output_dir=#{options[:output_dir]}"]
        }
        unless asciidoctor_validate(file, filename, out_opts)
          warn "Cannot continue compiling Asciidoctor document"
          abort
        end
        ::Asciidoctor.convert(file, out_opts)
      end

Asciidoctor is really not designed for passing variables into it. But if you call the above with out_opts[:log] = "LOG", it shows up in

https://github.com/metanorma/metanorma-standoc/blob/a16d7b909329842bf3ad32793349908c80ed4e56/lib/metanorma/standoc/converter.rb#L89

 def initialize(backend, opts)

as opts[:document].options[:log]. So, under def initialize(backend, opts), a = opts[:document].options[:log] and @log = a.

It also makes sense to pass the file location to the log on initialising it, not on writing it, as the filename may not be available by the time metanorma closes the file. I can make that change.

... Do you want me to start the scaffolding for this in metanorma-utils and metanorma-standoc?

@opoudjis
Copy link
Contributor

@CAMOBAP I will: metanorma/metanorma#350 . I will update you with the refactoring, but the idea will be for the log to be initialised in metanorma gem, not standoc.

Of course, you don't want a log out of this, you want:

  • non-zero exit code
  • print fatal error to the stdout

But the log will enable printing errors to stdout consistently (and saving them to disk, which has become critical), and because all messages are stored with their severities, we can track the occurrences of fatal errors.

opoudjis added a commit to metanorma/metanorma that referenced this issue Feb 21, 2024
@opoudjis
Copy link
Contributor

opoudjis commented Feb 21, 2024

OK:

You now only (only :-) need to modify metanorma and isodoc, but you will do the following:

  • I am generating the log and writing it to disk within Metanorma in the PR
  • You will need to write the test cases, and whatever else you want to do with the exit codes
  • The log is created as @log, and passed to standoc as options[:log] in the call to input_to_isodoc(file, filename, options = {})
  • The log is written to disk in clean_exit(), and that's where you would pick up any fatal errors and non-zero exit codes
  • There is currently no capture in logging of any errors in the isodoc suite. You would need to pass the log in as an argument to the isodoc call, but you can just rely on the existing tracking of errors, as long as you pass them into the log after Compiler::compile_file

@opoudjis
Copy link
Contributor

From metanorma/isodoc#314:

A request to implement --strict and --loose validation in metanorma-cli:

  • If strict, "die on the first error" (although I think that means severity 1, certainly not 2 or 3)
  • If loose, "suppresses the non-zero status on minor errors"

To realise this, we will need to agree on the levels of severity that trigger this behaviour.

@ronaldtse
Copy link
Contributor

Agree with @opoudjis on loose vs strict.

@opoudjis
Copy link
Contributor

opoudjis commented Mar 4, 2024

@CAMOBAP update on this please?

@CAMOBAP
Copy link
Contributor Author

CAMOBAP commented Mar 4, 2024

no update yet

@opoudjis
Copy link
Contributor

opoudjis commented Mar 4, 2024

I've merged the relevant PR in standoc into main, because it addressed a separate issue.

opoudjis added a commit to metanorma/metanorma that referenced this issue Mar 22, 2024
@opoudjis
Copy link
Contributor

I've now merged the PRs for metanorma and metanorma-utils as well: logs are now initialised and written to disk in metanorma, not metanorma-standoc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: 🌋 Urgent
Development

No branches or pull requests

3 participants