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

doc: Honor SOURCE_DATE_EPOCH for documentation timestamps. #4286

Merged
merged 2 commits into from Jun 2, 2022
Merged

doc: Honor SOURCE_DATE_EPOCH for documentation timestamps. #4286

merged 2 commits into from Jun 2, 2022

Conversation

Apteryks
Copy link
Contributor

Fixes #4262.

  • doc/userguide/ug2html.py (create_userguide): Do not embed
    timestamps.
  • src/robot/utils/robottime.py (get_timestamp_for_doc): New procedure.
  • src/robot/utils/init.py: Export it.
  • src/robot/libdocpkg/model.py (LibraryDoc.to_dictionary): Use it.
  • src/robot/libdocpkg/xmlwriter.py (LibdocXmlWriter._write_start):
    Likewise.
  • BUILD.rst: Document it.

@Apteryks Apteryks mentioned this pull request Mar 21, 2022
Closed
@Apteryks
Copy link
Contributor Author

OK, so this seems to have broken acceptance tests, due to slightly changing the timestamp format used in XML generated files. I'll try looking at it later.

@Apteryks Apteryks force-pushed the honor-source-data-epoch branch 4 times, most recently from 0750cc0 to b7d3242 Compare March 23, 2022 23:37
pekkaklarck
pekkaklarck requested changes May 27, 2022
View reviewed changes
Copy link
Member

@pekkaklarck pekkaklarck left a comment

Choose a reason for hiding this comment

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

Looks good but I think it's better to inline the timestamp generation code than have it in robot.utils that's part of our public API. Change to some tests seem unnecessary as well. See the more detailed comments for details.

BUILD.rst Outdated Show resolved Hide resolved
atest/robot/standard_libraries/operating_system/modified_time.robot Show resolved Hide resolved
src/robot/libdocpkg/xmlwriter.py Outdated
attrs = {'name': libdoc.name,
'type': libdoc.type,
'format': libdoc.doc_format,
'scope': libdoc.scope,
'generated': generated,
'generated': get_timestamp_for_doc(),
Copy link
Member

Choose a reason for hiding this comment

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

This changes the timestamp from current 2022-05-27T19:08:42Z to 2022-05-27T19:07:15+00:00. That ought to be the same thing, and mach xs:dateTime that we use in libdoc.xsd schema, but because external tools may depend on the exact format this makes the change backwards incompatible. It's not so big change it couldn't be done in RF 5.1, but need to still be mentioned in the release notes. I'll add a label to the issue so that we don't forget about it.

src/robot/libdocpkg/model.py Outdated
@@ -113,7 +113,7 @@ def to_dictionary(self):
'name': self.name,
'doc': self.doc,
'version': self.version,
'generated': get_timestamp(daysep='-', millissep=None),
'generated': get_timestamp_for_doc(),
Copy link
Member

Choose a reason for hiding this comment

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

This changes the current timestamp from 2022-05-27 22:07:15 to 2022-05-27T19:07:15+00:00. The old timestamp was naive and I guess it makes sense to have the timezone information there. The change is only in HTML output so it shouldn't cause real backwards compatibility problems. I don't particularly like the T separator in outputs targeted for humans, but don't care strongly about it.

Copy link
Member

Choose a reason for hiding this comment

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

Ooops, my comment was partly invalid. This changes also JSON spec files, not only HTML output. Thus this change is backwards incompatible the same way as the change to XML spec files. Also makes the T separator pretty much mandatory.

src/robot/utils/robottime.py Outdated Show resolved Hide resolved
@pekkaklarck pekkaklarck mentioned this pull request May 27, 2022
Closed
@pekkaklarck
Copy link
Member

One more thing to do is creating tests for this functionality. Need to add tests for JSON and XML specs where SOURCE_DATE_EPOCH is set to some know value before generating specs.

pekkaklarck
pekkaklarck requested changes Jun 1, 2022
View reviewed changes
Copy link
Member

@pekkaklarck pekkaklarck left a comment •
edited

Choose a reason for hiding this comment

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

Things to do:

  • Move the code to generate the timestamp from robot.utils to robot.libdocpkg to avoid it being part of the public API.
  • Revert changes to tests under atest/{robot,testdata}/standard_libraries unless they are actually needed. They don't seem to be related to this change and mostly make tests less specific.
  • Add test that SOURCE_DATE_EPOCH can be used to control the generation time in LIbdoc spec files. Should basically have same test for the generation time as normally but set the environment variable to some know value before running Libdoc. Then we should be able to validate that the exact generation time is what we expect. If you need help with tests, comment here or ping me on Slack.

src/robot/utils/robottime.py Outdated Show resolved Hide resolved
@Apteryks
Copy link
Contributor Author

Apteryks commented Jun 1, 2022

Things to do:

* Move the code to generate the timestamp from `robot.utils` to `robot.libdocpkg` to avoid it being part of the public API.

OK. That seems simple enough. I'll look into it.

* Revert changes to tests under `atest/{robot,testdata}/standard_libraries` unless they are actually needed. They don't seem to be related to this change and mostly make tests less specific.

These changes are actually needed, as I wrote here https://github.com/robotframework/robotframework/pull/4286/files#r883940643. Other adjustments are needed due to the format having changed slightly.

* Add test that `SOURCE_DATE_EPOCH` can be used to control the generation time in LIbdoc spec files. Should basically have same test for the generation time as normally but set the environment variable to some know value before running Libdoc. Then we should be able to validate that the exact generation time is what we expect. If you need help with tests, comment here or ping me on Slack.

Thanks, I'll look into adding an extra test here as well.

@Apteryks
Copy link
Contributor Author

Apteryks commented Jun 1, 2022

To clarify, the second commit adjusts tests so they pass when SOURCE_EPOCH_DATE is used while building the package.

pekkaklarck
pekkaklarck requested changes Jun 2, 2022
View reviewed changes
Copy link
Member

@pekkaklarck pekkaklarck left a comment

Choose a reason for hiding this comment

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

The created test is really complicated and needs to be simplified.

Would also be good to revert unrelated test changes. Or are they actually needed?

I also added some nitpickey comments about imports and function name, but I can change those after the merge myself as well.

src/robot/libdocpkg/xmlwriter.py Show resolved Hide resolved
src/robot/libdocpkg/output.py Outdated Show resolved Hide resolved
atest/robot/libdoc/spec_library.robot Show resolved Hide resolved
atest/robot/libdoc/spec_library.robot Outdated Show resolved Hide resolved
Maxim Cournoyer added 2 commits June 2, 2022 16:54
doc: Honor SOURCE_DATE_EPOCH for documentation timestamps.
72b6166 
Fixes robotframework#4262.

* doc/userguide/ug2html.py (create_userguide): Do not embed
timestamps.
* src/robot/libdocpkg/output.py (get_generation_time): New procedure.
* src/robot/libdocpkg/model.py (LibraryDoc.to_dictionary): Use it.
* src/robot/libdocpkg/xmlwriter.py (LibdocXmlWriter._write_start):
Likewise.
* atest/robot/libdoc/html_output.robot: Adjust regexp.
* atest/robot/libdoc/json_output.robot: Likewise.
* atest/robot/libdoc/libdoc_resource.robot
(Generated Should Be Defined): Adjust wildcard.
(Generated Should Be): New keyword.
* atest/robot/libdoc/spec_library.robot
(SOURCE_DATE_EPOCH is honored in Libdoc output): New test.
* BUILD.rst: Document it.
atest: Relax assumptions about the date.
602b14c 
Related to robotframework#4262.

* atest/robot/standard_libraries/operating_system/modified_time.robot
(Get Modified Time As Timestamp): Do not assume the current year
starts with '20'; use a regexp.
* atest/testdata/standard_libraries/builtin/get_time.robot
(Get Time As Seconds After Epoch): Relax arbitrary time boundaries.
(Get Time As Parts): Likewise.
* atest/testdata/standard_libraries/operating_system/modified_time.robot:
Adjust similarly.
@pekkaklarck pekkaklarck merged commit f8a9861 into robotframework:master Jun 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pekkaklarck pekkaklarck pekkaklarck approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Honor SOURCE_DATE_EPOCH environment variable when generating library documentation
2 participants
@Apteryks @pekkaklarck