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

Support esp-idf source tree not being Git repository #80

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

haileys
Copy link

@haileys haileys commented Sep 15, 2023

embuild currently expects the esp-idf source tree (set by EMBUILD_ESP_IDF_PATH) to be a Git repository. It errors out if this path is not a Git repository.

This has the unfortunate side effect of preventing esp-idf from being a Git submodule of another repository - this is how I'd like to configure my project, so I can manage esp-idf versions directly and have an easily accessible checkout.

This pull request does two things:

  • Add a new SourceTree enum with a single variant, Repository. There is a SourceTree::path method that delegates to git::Repository::worktree. It appears that this is the only method embuild currently calls on git::Repository

  • Adds a second SourceTree::Plain variant which holds a PathBuf directly. This variant is constructed if we fail to open a git repository at the esp-idf path.

I wonder if we could just remove the use of git::Repository entirely here, but I figure maybe there might be future uses planned, which is why I've opted for the enum variant approach in this PR. My hope is that this change makes embuild more flexible for different project configurations.

@haileys
Copy link
Author

haileys commented Sep 15, 2023

Oh! I see where those git repo methods are being called after all - in esp-idf-sys!

I prepared some changes for esp-idf-sys to update according to this change: esp-rs/esp-idf-sys@master...haileys:esp-idf-sys:esp-idf-no-git-repo

It's unfortunate that we'll need to make both of these changes in lockstep, but I can't think of a better way. esp-idf-sys is unfortunately currently coupled to Git repo support - but only really when using a managed repo, otherwise it only calls worktree().

If this change looks good, let me know and I'll open up a PR to esp-idf-sys with my linked changes.

src/espidf.rs Show resolved Hide resolved
@ivmarkov
Copy link
Collaborator

@N3xed would you have time to review and provide guidance?

Copy link
Collaborator

@N3xed N3xed left a comment

Choose a reason for hiding this comment

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

Thanks for the improvements.

Just some small things and then it's good to go.
Note also that CI is failing on cargo fmt.

src/espidf.rs Show resolved Hide resolved
src/espidf.rs Outdated Show resolved Hide resolved
src/espidf.rs Outdated Show resolved Hide resolved
src/espidf.rs Outdated Show resolved Hide resolved
src/espidf.rs Outdated Show resolved Hide resolved
src/espidf.rs Outdated Show resolved Hide resolved
src/espidf.rs Outdated Show resolved Hide resolved
@haileys
Copy link
Author

haileys commented Sep 18, 2023

Thank you for the review @N3xed! I have made all the requested changes!

@N3xed
Copy link
Collaborator

N3xed commented Sep 18, 2023

@haileys Changes look good! Just cargo fmt and maybe cargo clippy left. If you want you can already make a PR to esp-idf-sys with embuild patched to this branch.

@haileys
Copy link
Author

haileys commented Sep 18, 2023

cargo fmt applied!

@ivmarkov
Copy link
Collaborator

cargo fmt applied!

There's one clippy error that needs fixing too.

@ivmarkov
Copy link
Collaborator

@haileys I'm preparing a new release of embuild and esp-idf-sys. Can you address the remaining clippy error so I can merge this for the release? Thanks!

@haileys
Copy link
Author

haileys commented Sep 20, 2023

Whoops sorry for the delay, fixed that up now.

@ivmarkov
Copy link
Collaborator

@haileys Are you looking at the CI pipeline when you are pushing (not sure - I'm genuinely asking, as I don't remember if the CI pipeline is visible for first-time committers to the project)?

Anyway - you need to run cargo fmt again.

@ivmarkov
Copy link
Collaborator

ivmarkov commented Oct 3, 2023

@haileys Are you still interested in pushing this to completion? We are missing a cargo fmt in your PR.

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