Skip to content

Fix path to python native helpers#5617

Merged
deivid-rodriguez merged 1 commit intomainfrom
deivid-rodriguez/python-helpers-path
Sep 6, 2022
Merged

Fix path to python native helpers#5617
deivid-rodriguez merged 1 commit intomainfrom
deivid-rodriguez/python-helpers-path

Conversation

@deivid-rodriguez
Copy link
Copy Markdown
Contributor

@deivid-rodriguez deivid-rodriguez commented Sep 2, 2022

While working on python native helpers, I was running python/helpers/build after making changes to install native helpers to the proper location. However, my changes were not being picked up.

Turns out Dependabot was using /opt/python/helpers, the path where helpers are originally copied:

COPY --chown=dependabot:dependabot python/helpers /opt/python/helpers

Not the path where helpers are originally installed:

install_dir="$DEPENDABOT_NATIVE_HELPERS_PATH/python"

Initially I just fixed the path but thinking about it more, this file tree duplication seems unnecessary and confusing, specifically for interpreted language (where installed files are exactly the same as copied files).

Should we instead remove any file copying from build scripts and use the copied tree directly like it was already done for python?

@deivid-rodriguez
Copy link
Copy Markdown
Contributor Author

Initially I just fixed the path but thinking about it more, this file tree duplication seems unnecessary and confusing, specifically for interpreted language (where installed files are exactly the same as copied files).

Should we instead remove any file copying from build scripts any use the copied tree directly like it was already done for python?

After a bit more thought, I don't have great ideas here, and I wouldn't like to spend too much time on this, so I'll just leave this as is.

Let me know if I'm missing something!

@deivid-rodriguez deivid-rodriguez marked this pull request as ready for review September 2, 2022 10:40
@deivid-rodriguez deivid-rodriguez requested a review from a team as a code owner September 2, 2022 10:40
@jakecoffman
Copy link
Copy Markdown
Member

This has been annoying for a long time so it would be great to sort it out, thanks for taking it on @deivid-rodriguez!

One thing I'm curious about is if I run the CLI:

script/debug update go_modules rsc/quote --dry-run --updater-image=ghcr.io/dependabot/dependabot-updater:785959b2ed553e3c4ffdda4366e12237a713ad05 

We have two directories that seem confusingly identical:

dependabot@200e3450e402:~/dependabot-updater$ ls /opt/python/
helpers  lib  requirements.txt  run.py
dependabot@200e3450e402:~/dependabot-updater$ ls /opt/python/helpers/
build  lib  requirements.txt  run.py

If we don't need both it might go a long way to fixing confusion if they weren't duplicated.

@deivid-rodriguez
Copy link
Copy Markdown
Contributor Author

deivid-rodriguez commented Sep 2, 2022

Yes, they are indeed!

The way I understand it, /opt/python/helpers/ holds the "sources" for native helpers, while /opt/python holds the compiled versions installed at the final location. But this distinction does not make much sense for interpreted languages and results in this duplication.

I'm not sure what's best to do. One idea would be to eliminate the intermediate "source locations" (/opt/python/helpers) and build native helpers directly from our source tree.

@jakecoffman
Copy link
Copy Markdown
Member

@pavera any thoughts around this? I know you also dug into this at one point.

@pavera
Copy link
Copy Markdown
Contributor

pavera commented Sep 6, 2022

The duplication is confusing, but it's how all the ecosystems work, so having one be different feels even more confusing to me. I think the larger proposed change would mean moving a bunch of copy commands from the various build scripts into the Dockerfile and would make the dockerfile aware of helper contents for each specific ecosystem whereas currently this is abstracted away by the build scripts. Since some ecosystems produce different output in /opt/<ecosystem>/ vs their respective /opt/<ecosystem>/helpers content I think leaving this as is may be the least bad option?

If we wanted to make a larger change maybe breaking the "source" and "dist" trees out would make this distinction more clear? IE /opt/helpers-source/<ecosystem>/ is where we copy the helper source for each ecosystem in the Dockerfile, then the build scripts all target /opt/<ecosystem>/ and all code targets /opt/<ecosystem>/. Might make it easier to spot/avoid issues like the one this PR fixes.

Copy link
Copy Markdown
Contributor

@pavera pavera left a comment

Choose a reason for hiding this comment

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

I think this fix is the smallest most correct fix currently possible.

@deivid-rodriguez
Copy link
Copy Markdown
Contributor Author

The duplication is confusing, but it's how all the ecosystems work, so having one be different feels even more confusing to me.

Agreed! Any better solution to this would need to be applied consistently across ecosystems.

I think the larger proposed change would mean moving a bunch of copy commands from the various build scripts into the Dockerfile and would make the dockerfile aware of helper contents for each specific ecosystem whereas currently this is abstracted away by the build scripts. Since some ecosystems produce different output in /opt// vs their respective /opt//helpers content I think leaving this as is may be the least bad option?

Actually my idea is to not COPY the sources at all to /opt. Instead, run each native helper build script directly from the original repo structure.

I'll merge this for now, in any case!

@deivid-rodriguez deivid-rodriguez force-pushed the deivid-rodriguez/python-helpers-path branch 2 times, most recently from f8db900 to 71d2782 Compare September 6, 2022 18:47
@deivid-rodriguez deivid-rodriguez force-pushed the deivid-rodriguez/python-helpers-path branch from 71d2782 to 3d4b9df Compare September 6, 2022 19:17
@deivid-rodriguez deivid-rodriguez merged commit 2601f1f into main Sep 6, 2022
@deivid-rodriguez deivid-rodriguez deleted the deivid-rodriguez/python-helpers-path branch September 6, 2022 19:36
@mctofu mctofu mentioned this pull request Sep 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants