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

Update GIT-INFO.md for perl version requirements. #14112

Closed
wants to merge 6 commits into from

Conversation

areitz
Copy link
Contributor

@areitz areitz commented Jul 5, 2024

Per ChatGPT, the <:crlf discipline was added to open() in Perl 5.8.0. I tried running scripts/managen on a machine that has Perl 5.6.1 and got an error:

Unknown open() mode ':<crlf' at ../../scripts/managen line 777.

While I was in there, I also made the list of requirements a valid Markdown list, so it displays properly when viewed in GitHub.

Before:

Screenshot 2024-07-05 at 6 45 37 PM

After:

Screenshot 2024-07-05 at 6 45 23 PM

I made these separate commits, so you can not take either change if you wish.

Thanks!

Per ChatGPT, the `<:crlf` discipline was added to `open()` in Perl 5.8.0. I tried running `scripts/managen` on a machine that has Perl 5.6.1 and got an error:

```
Unknown open() mode ':<crlf' at ../../scripts/managen line 777.
```
Convert list of requirements to a valid list in Markdown.
@areitz areitz marked this pull request as ready for review July 5, 2024 23:47
GIT-INFO.md Outdated Show resolved Hide resolved
@vszakats
Copy link
Member

vszakats commented Jul 6, 2024

FWIW, another GPT gives this answer to the same question:
"The :crlf discipline for the open() function in Perl was added in Perl 5.10.0, which was released on December 18, 2007. This feature allows Perl to automatically handle line ending conversions when reading or writing files, making it easier to work with text files that may have been created on different platforms with different line ending conventions."

To "are you sure?", it replies with a different version:
"I apologize for the confusion in my previous responses. The :crlf discipline for the open() function in Perl was actually added in Perl 5.14.0, not Perl 5.10.0 as previously mentioned. Perl 5.14.0 was released on May 14, 2011. This feature allows Perl to handle line ending conversions when reading or writing files, simplifying the process of working with text files across different platforms. Thank you for pointing out the error, and I appreciate your understanding."

Is this somewhere documented in a canonical source?

@dfandrich
Copy link
Contributor

dfandrich commented Jul 6, 2024 via email

Excellent feedback is excellent.

Co-authored-by: Viktor Szakats <vszakats@users.noreply.github.com>
@areitz
Copy link
Contributor Author

areitz commented Jul 6, 2024

I did some digging, but didn't study the perl source code too carefully. It seems like this is the commit that adds the <:crlf syntax:

Perl/perl5@95c70f2

Looking at the release timeline, that is after Perl 5.6.0 but before 5.8.0:

https://en.wikipedia.org/wiki/Perl_5_version_history

I think this predates git, so I'm not sure how I'd track down if this change was backported to the 5.6.x train or stayed in the 5.8.x train. But the release notes for 5.8.0 say:

New PerlIO implementation

And I believe this feature is part of PerlIO, so I'd believe that it first came in Perl 5.8.0.

@areitz
Copy link
Contributor Author

areitz commented Jul 6, 2024

The canonical location for version information is docs/INTERNALS.md. I didn't
realize there were some versions duplicated in GIT-INFO.md and I don't think we
should bother keeping both. GIT-INFO.md should probably defer to
docs/INTERNALS.md for versions if it's going to mention specific requirements
at all.

Thanks! I made that update, but I'm wondering if GIT-INFO.md is needed at all?

@dfandrich
Copy link
Contributor

dfandrich commented Jul 6, 2024 via email

GIT-INFO.md Outdated
Comment on lines 30 to 31
See [docs/INTERNALS.md][0] for information on the requirements that you'll
need or `autoreconf` and `configure` (not `buildconf.bat`) to work.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
See [docs/INTERNALS.md][0] for information on the requirements that you'll
need or `autoreconf` and `configure` (not `buildconf.bat`) to work.
See [docs/INTERNALS.md][0] for requirement details.

... and I think the next paragraph can be completely removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! How does this look now?

@areitz areitz requested a review from bagder July 8, 2024 16:48
@bagder bagder closed this in 400717e Jul 8, 2024
@bagder
Copy link
Member

bagder commented Jul 8, 2024

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

4 participants