Skip to content

Update the RECORD entry when rewriting the shebang line in a script#11052

Merged
sbidoul merged 1 commit intopypa:mainfrom
SpecLad:fix-script-record-hash
Jun 26, 2022
Merged

Update the RECORD entry when rewriting the shebang line in a script#11052
sbidoul merged 1 commit intopypa:mainfrom
SpecLad:fix-script-record-hash

Conversation

@SpecLad
Copy link
Copy Markdown
Contributor

@SpecLad SpecLad commented Apr 20, 2022

The code to do this already exists in get_csv_rows_for_installed, but it's broken due to inconsistent usage of the _fs_to_record_path function. When we build the dictionary of installed files, we call it with a base directory, while when build the set of modified files, we call it without a base directory. As a result, the values of installed do not match the elements of changed, and get_csv_rows_for_installed fails to identify the rows that should be updated.

Fix this by ensuring that _fs_to_record_path is always called with a base directory. _record_to_fs_path also needs a a base directory parameter to be able to transform the path back into an absolute path, so add one.

Fixes #10744

@SpecLad SpecLad force-pushed the fix-script-record-hash branch from 4054130 to e99ea9e Compare April 20, 2022 23:06
Copy link
Copy Markdown
Member

@uranusjr uranusjr left a comment

Choose a reason for hiding this comment

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

Makes sense to me. We should probably switch to installer eventually but this is small enough a change to go in now.

@github-actions github-actions bot added the needs rebase or merge PR has conflicts with current master label Jun 12, 2022
@SpecLad SpecLad force-pushed the fix-script-record-hash branch from e99ea9e to b282e58 Compare June 15, 2022 14:32
@pypa-bot pypa-bot removed the needs rebase or merge PR has conflicts with current master label Jun 15, 2022
@SpecLad
Copy link
Copy Markdown
Contributor Author

SpecLad commented Jun 15, 2022

Rebased to fix merge conflicts. There's one test failure now, but I think that's due to a transient network problem.

@sbidoul
Copy link
Copy Markdown
Member

sbidoul commented Jun 24, 2022

@SpecLad there was a CI error, likely a transient. Could you rebase again to be sure everything is green ?

The code to do this already exists in `get_csv_rows_for_installed`, but it's
broken due to inconsistent usage of the `_fs_to_record_path` function. When
we build the dictionary of installed files, we call it with a base
directory, while when build the set of modified files, we call it without a
base directory. As a result, the values of `installed` do not match the
elements of `changed`, and `get_csv_rows_for_installed` fails to identify
the rows that should be updated.

Fix this by ensuring that `_fs_to_record_path` is always called with a base
directory. `_record_to_fs_path` also needs a a base directory parameter to
be able to transform the path back into an absolute path, so add one.
@SpecLad SpecLad force-pushed the fix-script-record-hash branch from b282e58 to e4cd6da Compare June 25, 2022 01:13
@uranusjr
Copy link
Copy Markdown
Member

The ReadTheDocs job did complete, but reporting on GitHub seems stuck for whatever reason. Admins can still merge though.

@pradyunsg pradyunsg closed this Jun 25, 2022
@pradyunsg pradyunsg reopened this Jun 25, 2022
@sbidoul sbidoul merged commit 340054a into pypa:main Jun 26, 2022
@sbidoul
Copy link
Copy Markdown
Member

sbidoul commented Jun 26, 2022

Thank you, @SpecLad !

@SpecLad SpecLad deleted the fix-script-record-hash branch June 26, 2022 13:43
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 13, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

RECORD size and hash do not reflect rewritten shebangs

5 participants