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.

Sign up for GitHub

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

Jump to bottom

High level logger FA definitions #1440

Merged
merged 15 commits into from Aug 20, 2020
Merged

High level logger FA definitions #1440

merged 15 commits into from Aug 20, 2020

Conversation

ethouris
Copy link
Collaborator

@ethouris ethouris commented Aug 5, 2020 •
edited

This replaces the dispersed definitions of the defined logging functional areas by:

  • the code that generates them basing on the central definition
  • the central definition itself

The "central definition" is in the scripts/generate-logging-defs.tcl together with:

  1. The model of how these definitions expand to a C++ code required in particular place.
  2. The definition of 4 files to be generated:
  • srtcore/logger_defs.h - provides declarations of loggers and header for default config
  • srtcore/logger_defs.cpp - provides definitions of loggers
  • srtcore/logger_defaults.cpp - provides default configuration of the loggers to make them all set
  • apps/logsupport_appdefs.cpp - provides name-to-symbol bindings for applications

The only exception is in srt.h because this is an API file, so generation of an API file wasn't considered user friendly.

Changes in the FA definitions now require only:

  1. Adding or modifying the definition in the loggers list in generate-logging-defs.tcl file
  2. Regenerating by running generate-logging-defs.tcl script
  3. Pasting the definitions stored in srt.inc.h file into srt.h in the right location.
  4. Remember to check-in all modified files!

maxsharabayko
maxsharabayko reviewed Aug 5, 2020
View reviewed changes
Copy link
Collaborator

@maxsharabayko maxsharabayko left a comment

Choose a reason for hiding this comment

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

Please provide a description of what this PR adds to guide the review process.

@ethouris ethouris self-assigned this Aug 10, 2020
@ethouris ethouris added [core] Area: Changes in SRT library core Impact: Development Type: Maintenance Work required to maintain or clean up the code labels Aug 10, 2020
@ethouris ethouris added this to Backlog in Development via automation Aug 10, 2020
@ethouris ethouris added this to the v1.5.0 - Sprint 20 milestone Aug 10, 2020
@mbakholdina mbakholdina modified the milestones: v1.5.0 - Sprint 20, v1.5.0 - Sprint 21 Aug 10, 2020
@ethouris ethouris moved this from Backlog to Needs Review in Development Aug 11, 2020
@maxsharabayko maxsharabayko mentioned this pull request Aug 18, 2020
Closed
maxsharabayko
maxsharabayko reviewed Aug 19, 2020
View reviewed changes
Copy link
Collaborator

@maxsharabayko maxsharabayko left a comment

Choose a reason for hiding this comment

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

  1. Script generate-logging-defs.tcl usage help or guidelines seem to be missing.
  2. The script generates also apps/logsupport_appdefs.cpp file. I understand the reason. But it is just a bit different area: library and application. At least it should be an option whether to generate that file or not. Maybe SRT API should provide the list of FAs not as a set of definitions, but also as a list?
  3. Print the list of FAs in srt-live-transmit.
  4. Extra blank lines (see inline code comment).

srtcore/core.cpp Outdated Show resolved Hide resolved
apps/logsupport_appdefs.cpp Outdated
Comment on lines 1 to 4


/*
* SRT - Secure, Reliable, Transport
Copy link
Collaborator

Choose a reason for hiding this comment

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

These extra blank lines could be removed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sadly, quite a big part of the code in the script is to deal with spaces and indents. And it continues.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If it is too hard to delete - no problem. Let it stay.

@ethouris ethouris added the [docs] Area: Improvements or additions to documentation label Aug 19, 2020
maxsharabayko
maxsharabayko reviewed Aug 19, 2020
View reviewed changes
docs/Developers.md Outdated
@@ -96,6 +97,90 @@ The SRT installation has the following folders:
* *test - unit tests for the library.
* *testing - the folder contains applications used during development: `srt-test-live`, `srt-test-file`, etc. Use `-DENABLE_TESTING=ON` to include in the build.

## Rules
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
## Rules
## Coding Rules

docs/Developers.md Outdated
@@ -6,6 +6,7 @@
* [Building SRT](#building-srt)
* [Project Structure](#project-structure)
* [Coding Rules](#rules)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
* [Coding Rules](#rules)
* [Coding Rules](#coding-rules)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That was yours! :D

Ok, I'll fix the target stub section as well.

docs/Developers.md Outdated
@@ -6,6 +6,7 @@
* [Building SRT](#building-srt)
* [Project Structure](#project-structure)
* [Coding Rules](#rules)
* [Generated files](#generated-files)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would rather move it to the very end of the document as it is a very rare case that someone (especially external) would use those scripts.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right, of course.

ethouris
ethouris commented Aug 19, 2020
View reviewed changes
docs/Developers.md Outdated

* `srtcore/logger_default.cpp`: contains setting of all FA as enabled
* `srtcore/logger_defs.h` and `srtcore/logger_defs.cpp`: declares/defiones logger objects
* `apps/logsupport_appdefs.cpp`: Provides string-to-symbol bindings for the applications
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Forgot the srt.inc.h.

ethouris
ethouris commented Aug 19, 2020
View reviewed changes
docs/Developers.md Outdated
Currently generated files are:

* `srtcore/logger_default.cpp`: contains setting of all FA as enabled
* `srtcore/logger_defs.h` and `srtcore/logger_defs.cpp`: declares/defiones logger objects
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

defines

@ethouris ethouris removed the [docs] Area: Improvements or additions to documentation label Aug 20, 2020
@maxsharabayko maxsharabayko merged commit 98e86d1 into Haivision:master Aug 20, 2020
Development automation moved this from Needs Review to Done Aug 20, 2020
@mbakholdina mbakholdina modified the milestones: v1.5.0 - Sprint 21, v1.4.2 Oct 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maxsharabayko maxsharabayko maxsharabayko left review comments

@stevomatthews stevomatthews Awaiting requested review from stevomatthews

Assignees

@ethouris ethouris

Labels
[core] Area: Changes in SRT library core Type: Maintenance Work required to maintain or clean up the code
Projects
No open projects
Development
  
Done
Milestone
v1.4.2
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@ethouris @maxsharabayko @mbakholdina