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

Fix relative targetpath bug #3539

Closed
wants to merge 1 commit into from
Closed

Conversation

sinwoobang
Copy link

Bug Fix: Path Resolution for Local File System Imports

TL;DR

  • Before: When using local files, you were building paths starting from where the command was run
  • After: Now you build paths starting from where the files are imported from, just like we already do with git
  • The fix: You explicitly resolve the target path using the import path as the base directory

The key change was making sure we use the import location as our "starting point" when figuring out where to put files, regardless of whether we're importing from local files or git.

Issue

The export command was failing for local filesystem imports while working correctly for git imports. When using local filesystem imports, relative output paths were incorrectly resolved, causing errors when trying to export to paths outside the context directory.

For example, this local filesystem import failed:

export -o apivendor --exclude-imports ../local-vendor-protos --path package/project/common

With error:

Failure: ../cur_dir/package/project/common: is outside the context directory

While the equivalent git import worked as expected:

export -o apivendor --exclude-imports ssh://git/local-vendor-protos.git --path package/project/common

Root Cause

The path resolution logic for local filesystem imports was not implementing the same correct behavior as git imports. Local filesystem imports were incorrectly using the current directory as the base for resolving relative paths.

Fix

Updated the local filesystem path resolution logic to match the correct behavior already implemented in git imports. The path is now properly resolved as ../apivendor/package/project/common for both import types.

@CLAassistant
Copy link

CLAassistant commented Dec 13, 2024

CLA assistant check
All committers have signed the CLA.

@bufdev
Copy link
Member

bufdev commented Dec 13, 2024

@doriable can you check this out and make sure its (1) right (2) unix/windows independent?

@doriable
Copy link
Member

Hello, thanks for submitting this PR! So, this is actually the intended behaviour of paths and buf commands.

For local inputs, paths provided to flags are always relative to the execution context of the command. For git inputs, it is considered relative to the input directory, which in your case is the top-level of the repository, but this can be set with the subdir (docs: https://buf.build/docs/reference/inputs/#git).

I can help with adjusting your workflows as needed, feel free to file an issue and/or reach out on Slack.

@doriable doriable closed this Dec 16, 2024
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.

4 participants