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

[JENKINS-69195] force update of the files so they are checked out with eol=lf #6948

Closed
wants to merge 1 commit into from

Conversation

jtnord
Copy link
Member

@jtnord jtnord commented Aug 1, 2022

See JENKINS-69195.

yarn ci:lint fails if the less files have non unix line ends. Whilst .gitattributes specifies the line endings for .less files now - nothing happens for their line endings unless the file is updated (as some of the files are 3 years since the last change that would be a very old checkout)

This adds a comment that then ensures the files will be newer and git will transform the line ending on windows if anyone is fetching into an existing repo as opposed to cloning something new.

Proposed changelog entries

  • Entry 1: Issue, Human-readable Text
  • ...

Proposed upgrade guidelines

N/A

Submitter checklist

  • (If applicable) Jira issue is well described
  • Changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developer, depending on the change) and are in the imperative mood. Examples
    • Fill-in the Proposed changelog entries section only if there are breaking changes or other changes which may require extra steps from users during the upgrade
  • Appropriate autotests or explanation to why this change has no tests
  • New public classes, fields, and methods are annotated with @Restricted or have @since TODO Javadoc, as appropriate.
  • New deprecations are annotated with @Deprecated(since = "TODO") or @Deprecated(forRemoval = true, since = "TODO") if applicable.
  • For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

@mention

Maintainer checklist

Before the changes are marked as ready-for-merge:

  • There are at least 2 approvals for the pull request and no outstanding requests for change
  • Conversations in the pull request are over OR it is explicit that a reviewer does not block the change
  • Changelog entries in the PR title and/or Proposed changelog entries are accurate, human-readable, and in the imperative mood
  • Proper changelog labels are set so that the changelog can be generated automatically
  • If the change needs additional upgrade steps from users, upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the PR title. (example)
  • If it would make sense to backport the change to LTS, a Jira issue must exist, be a Bug or Improvement, and be labeled as lts-candidate to be considered (see query).

@jtnord jtnord added the skip-changelog Should not be shown in the changelog label Aug 1, 2022
@jtnord jtnord marked this pull request as draft August 1, 2022 13:07
@jtnord jtnord marked this pull request as ready for review August 1, 2022 13:10
Copy link
Member

@basil basil left a comment

Choose a reason for hiding this comment

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

I do not think there is a need to force a refresh of checked out files through changes to the checked-in sources when one can merely follow the instructions given at Refreshing a repository after changing line endings. If following those instructions works, then I would prefer to close this PR.

@jtnord
Copy link
Member Author

jtnord commented Aug 1, 2022

I do not think there is a need to force a refresh of checked out files through changes to the checked-in sources when one can merely follow the instructions given at Refreshing a repository after changing line endings. If following those instructions works, then I would prefer to close this PR.

well yes, deleting everything except the .git directory and then reseting or otherwise will fix it but there are a couple of issues with that as a fix.

  1. it really needs you to know that the error is due to the line endings to begin with. The output from maven is Failed to run task: 'yarn lint:ci' failed. org.apache.commons.exec.ExecuteException: Process exited with an error: 2 (Exit value: 2) there is nothing here to tell you it is due to line endings. (if you know the output is in a checkstyle file then you can gain an inkling if you open the file in something that either understands XML (it is just a single long line) or checkstyle output).

  2. the fix is interim, and it does not persist in the repository and can be undone by going backwards (and then forwards) in time. For example if you are git bisecting then the conversion will be undone by:
    2a. going backwards to an older version of the file where /gitattributes is not setting the line end and the file(s) have been modified
    2b. then going forwards to where the file(s) have been changed but before the .gitattributes has been updated. (files will be CRLF)
    2c. then going forward in time where the .gitattributes are present - but the file contents are unchanged - leaving them with a a CRLF line end, and the "fix" has been undone.

Additionally there is nothing in git status (2.35.1) that says the files are changed:

(after going back in time to an older version and then forward twice ending on master) :

> git rev-parse HEAD
c1b233f5e1748b851d5b1170a82f920c7a77eb9c

> file src/main/less/*
src/main/less/abstracts:              directory
src/main/less/base:                   directory
src/main/less/form:                   directory
src/main/less/modules:                directory
src/main/less/pages:                  directory
src/main/less/pluginSetupWizard.less: ASCII text
src/main/less/simple-page.less:       ASCII text, with CRLF line terminators
src/main/less/styles.less:            ASCII text

> git status
On branch master
Your branch is up to date with 'origin/master'.

Untracked files:
  (use "git add <file>..." to include in what will be committed)
        testing/

nothing added to commit but untracked files present (use "git add" to track)

> 

If I could think of or knew something that actually needed to be changed in these files rather than leaving a bogus comment then I would happily adjust the PR to take those changes as opposed to a somewhat bogus comment.

@basil
Copy link
Member

basil commented Aug 1, 2022

it really needs you to know that the error is due to the line endings to begin with. The output from maven is Failed to run task: 'yarn lint:ci' failed. org.apache.commons.exec.ExecuteException: Process exited with an error: 2 (Exit value: 2) there is nothing here to tell you it is due to line endings. (if you know the output is in a checkstyle file then you can gain an inkling if you open the file in something that either understands XML (it is just a single long line) or checkstyle output).

That seems like a separate problem. A local Maven build shouldn't be using lint:ci; something named lint:ci should only be invoked from CI, where the lint results would be visible via Jenkins. Local runs should not be using lint:ci and should print the results on the terminal.


-0 from me for adding these comments if there is a local workaround.

@jtnord
Copy link
Member Author

jtnord commented Aug 1, 2022

That seems like a separate problem. A local Maven build shouldn't be using lint:ci; something named lint:ci should only be invoked from CI, where the lint results would be visible via Jenkins. Local runs should not be using lint:ci and should print the results on the terminal.

introduced unconditionally in #6894 as far as I can tell @timja?


as git-bisect is impacted I do not believe there is an acceptable local workaround, so leaving open for other opinions or suggestions.

@timja
Copy link
Member

timja commented Aug 1, 2022

introduced unconditionally in #6894 as far as I can tell @timja?

That change re-introduced a local option (yarn lint), the change that removed the read-able local version was:
#6817

It could conditionally swap between lint:ci and lint by using a profile activated by existence of the CI environment variable I assume

@basil basil added the proposed-for-close There is no consensus about feasibility of this PR, and maintainers do not see a way forward for it label Aug 30, 2022
@basil
Copy link
Member

basil commented Aug 30, 2022

With one -0 vote and no positive votes, I am marking this as proposed for close.

@basil
Copy link
Member

basil commented Sep 13, 2022

With one -0 vote and no positive votes in over 1 month, I am closing this PR.

@basil basil closed this Sep 13, 2022
@basil
Copy link
Member

basil commented Apr 10, 2023

the change that removed the read-able local version was: #6817

JENKINS-71021 FTR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
proposed-for-close There is no consensus about feasibility of this PR, and maintainers do not see a way forward for it skip-changelog Should not be shown in the changelog
Projects
None yet
3 participants