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

logs now report as pvgroups are added to pvl logs (fixes #4914) #4990

Merged
merged 8 commits into from
Jul 18, 2022

Conversation

Kelvinrr
Copy link
Collaborator

@Kelvinrr Kelvinrr commented Jul 13, 2022

Description

Pvl logs were being reported at the end of the programs execution which is a problem for apps that run for a long time. This changes the apps to report as groups are added.

This changes Application::Log to work with or without an iApp instance. Plus, a new function addLogGroup that adds the PVL group as usual but also calls Application::Log on the object.

Tried to catch all the converted apps using a recursive grep for Pvl appLog, Pvl results, Pvl log in main.cpps. I think I got all or at least a vast majority of them.

Application::Log calls were removed in main.cpps of converted tests as they're no longer needed. Left Application::GuiLog calls were left alone. Getsn was also left alone since it had very specific logging code in both the main and callable.

Related Issue

fixes #4914

Motivation and Context

Long running apps like jigsaw were not getting realtime updates.

How Has This Been Tested?

Jenkins tests, if nothing broke no new test failures should appear.

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Documentation change (update to the documentation; no code change)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have read and agree to abide by the Code of Conduct
  • I have read the CONTRIBUTING document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • I have added myself to the .zenodo.json document.
  • I have added any user impacting changes to the CHANGELOG.md document.

Licensing

This project is mostly composed of free and unencumbered software released into the public domain, and we are unlikely to accept contributions that are not also released into the public domain. Somewhere near the top of each file should have these words:

This work is free and unencumbered software released into the public domain. In jurisdictions that recognize copyright laws, the author or authors of this software dedicate any and all copyright interest in the software to the public domain.

  • I dedicate any and all copyright interest in this software to the public domain. I make this dedication for the benefit of the public at large and to the detriment of my heirs and successors. I intend this dedication to be an overt act of relinquishment in perpetuity of all present and future rights to this software under copyright law.

@Kelvinrr
Copy link
Collaborator Author

I'll update the test writing docs on the wiki after this is merged

@AustinSanders AustinSanders self-requested a review July 13, 2022 17:25
Copy link
Contributor

@AustinSanders AustinSanders left a comment

Choose a reason for hiding this comment

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

Overall I think it looks good. I can't find PvlLog.h, so I'm not sure what it contains. I hard a hard time understanding which files needed PvlLog.h imports as opposed to simple Pvl imports.

isis/src/base/apps/stretch/stretch_app.h Outdated Show resolved Hide resolved
Copy link
Contributor

@jessemapel jessemapel left a comment

Choose a reason for hiding this comment

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

Also needs a CHANGELOG entry

isis/src/base/objs/Application/Application.h Outdated Show resolved Hide resolved
@@ -12,6 +12,7 @@ find files of those names at the top level of this repository. **/
#include "IString.h"
#include "Message.h"
#include "PvlFormat.h"
#include "Application.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not super happy with PvlObject being dependent on Application, but I think this is the cleanest solution.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I'm not super keen on it either.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is spooky for integrating the ISIS Blob work next year. Likely requiring that Application and everything associated with it get pulled into that submodule within ISIS

Copy link
Contributor

Choose a reason for hiding this comment

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

Based on some discussion this morning. It's not ideal, but this solution is still good and the best one we have right now. We will need to come back and re-address some of the architecture next year so we're just adding some tech debt for us to deal with next year.

@Kelvinrr Kelvinrr changed the title logs now report as pvgroups are added to lvl logs (fixes #4914) logs now report as pvgroups are added to pvl logs (fixes #4914) Jul 15, 2022
@Kelvinrr
Copy link
Collaborator Author

results from local tests:

The following tests FAILED:
        543 - FitsToJson.FitsConversion (Subprocess aborted)
        623 - TempTestingFiles.UnitTestImagePolygonCross (Failed)
        853 - LronaccalDefault.FunctionalTestsLronaccal (Failed)
        854 - LronaccalNear.FunctionalTestsLronaccal (Failed)
        855 - LronaccalPair.FunctionalTestsLronaccal (Failed)

They're all weird "cant find data file" type errors, most likely not related to these changes at all.

@Kelvinrr
Copy link
Collaborator Author

Kelvinrr commented Jul 15, 2022

Gonna rerun all tests after that ctest PR was merged and double check everything is fine.

@Kelvinrr
Copy link
Collaborator Author

this time I got:

The following tests FAILED:
	520 - FitsToJson.FitsConversion (Subprocess aborted)
	600 - TempTestingFiles.UnitTestImagePolygonCross (Failed)
	899 - TestKernelDb.TestKernelsSmithOffset (Failed)

520 - some weird QT assertion error within the QT lib, most likely something weird with the version of QT my env picked up
600 - that tests seems to have a bad path for it's data? It doesn't like that my build dir is outside the repo it seems.
899 - weird CK error, seems like the same as 600

Either way, these don't seem to have anything to do with my changes and more to do with my build dir and env on prog28.

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.

Apps converted to a callable no longer log in real time
4 participants