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

Update to esp-idf 5.2.2 #56

Merged
merged 8 commits into from
Aug 26, 2024
Merged

Update to esp-idf 5.2.2 #56

merged 8 commits into from
Aug 26, 2024

Conversation

cortex
Copy link
Contributor

@cortex cortex commented Jun 20, 2024

No description provided.

@anpin
Copy link

anpin commented Jun 21, 2024

Hi, I have just tried your PR and found that idf.py --version still reports to be at 5.1.3

idf.py --version
fatal: not a git repository: '/nix/store/6fhgra3f7pmzljs1gz20ysv8ixq33il1-esp-idf-v5.2.2/.git'
WARNING: Git version unavailable, reading from source
ESP-IDF v5.1.3

pkgs/esp-idf/default.nix Outdated Show resolved Hide resolved
pkgs/esp-idf/default.nix Outdated Show resolved Hide resolved
Include hash for 5.2.2
@nagy
Copy link

nagy commented Jun 24, 2024

btw, there is another similar PR to this one #47

@cortex
Copy link
Contributor Author

cortex commented Jun 24, 2024

btw, there is another similar PR to this one #47

Thanks for pointing this out! I merged the changes into this branch, though I didn't bump in to these problems.

@cortex
Copy link
Contributor Author

cortex commented Jun 24, 2024

Hi, I have just tried your PR and found that idf.py --version still reports to be at 5.1.3

idf.py --version
fatal: not a git repository: '/nix/store/6fhgra3f7pmzljs1gz20ysv8ixq33il1-esp-idf-v5.2.2/.git'
WARNING: Git version unavailable, reading from source
ESP-IDF v5.1.3
`

@anpin Does it work for you with the latest changes?

@nagy
Copy link

nagy commented Jun 24, 2024

though I didn't bump in to these problems.

I did, that's how I found that PR. You can try to run this to see if your branch is able to build basic examples:

nix-build -E "with import <nixpkgs> {overlays=[(import ./overlay.nix)];}; callPackage ./tests/build-idf-examples.nix {}"

@cortex
Copy link
Contributor Author

cortex commented Jun 25, 2024

though I didn't bump in to these problems.

I did, that's how I found that PR. You can try to run this to see if your branch is able to build basic examples:

nix-build -E "with import <nixpkgs> {overlays=[(import ./overlay.nix)];}; callPackage ./tests/build-idf-examples.nix {}"

Thank you! With my latest change this build successfully for me at least

@nagy
Copy link

nagy commented Jun 25, 2024

One more thing I noticed is that, in that example file, there is still:

  buildsNameList = pkgs.lib.attrsets.cartesianProductOfSets {
    target = [ "esp32" "esp32c3" "esp32s2" "esp32s3" "esp32c6" "esp32h2" ];
    example = [ "get-started/hello_world" ];
  };

https://github.com/Lindboard/nixpkgs-esp-dev/blob/master/tests/build-idf-examples.nix#L36-L39

With idf 5.2 it looks like we are consolidating the esps2 and esps3 into the esp compiler. So maybe those two extra entries are no longer needed anymore in that test, but I am not sure.

@@ -38,6 +38,7 @@ let
rev = rev;
sha256 = sha256;
fetchSubmodules = true;
leaveDotGit = true;
Copy link

Choose a reason for hiding this comment

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

Be aware, that you also need to change the hash, if you decide to leave the .git directory in there.

@mksafavi
Copy link
Contributor

Hi.
Is there anything blocking this?

@mirrexagon
Copy link
Owner

Probably just me having time to review what's been done and merge this - I'll try to do that in the next day or two.

@saippua
Copy link

saippua commented Aug 4, 2024

112c9d1 what is the purpose of the version.txt file? The commit description makes me think its related to the issue that idf.py cannot find the version from git, because the nix store's git repository doesn't retain any tags. But I don't see how the version.txt file would help (and alas, I'm still getting that pesky warning from idf.py)

Honestly I haven't figured out how to get rid of the warning. Something like your earlier commit where you init a git repo inside the store directory, then manually adding the tag there could work, however it doesn't seem like a very stable solution.

On the other hand, maybe the problem is on the esp-idf side. Why should it throw unnecessary stderr warnings when it can't find the version through git tags, when it just falls back to getting the version from cmake, which should probably be the default anyway?

In any case, I've read elsewhere that leaveDotGit should be avoided as the .git folder is not stable. Not sure if that's true but here's the thread:
https://discourse.nixos.org/t/keep-git-folder-in-when-fetching-a-git-repo/8590/7

@saippua
Copy link

saippua commented Aug 4, 2024

I would also recommend something like this: 0298b4e

esp-idf checks that the python executable matches the IDF_PYTHON_ENV_PATH variable, which it won't if you set the env var to the symbolic link instead of the actual executable path.
see: https://sourcegraph.com/github.com/espressif/esp-idf/-/blob/tools/idf.py?L110-114

@saippua
Copy link

saippua commented Aug 8, 2024

Regarding the leaveDotGit option, I can now confirm that I've experienced some issues when it's set to true. I was trying to build the derivation on a different system but kept getting error: hash mismatch in fixed-output derivation, and the received hash kept changing every time.

Here's the relevant nixpkgs issue.
NixOS/nixpkgs#8567

@mirrexagon
Copy link
Owner

mirrexagon commented Aug 22, 2024

I also have noticed that leaveDotGit makes the output of fetchGit unstable, and yeah, in this case trying to build this branch causes a hash mismatch. @cortex For now, can we undo the leaveDotGit and the version.txt changes? Is that a problem for anyone? In my experience, the warning can just be ignored and everything works well enough.

I've been mostly ignoring trying to deal with the version stuff (#5) since I don't know enough about how the IDF checks versions and it doesn't cause me problems to have the warning.

@mrene mrene mentioned this pull request Aug 25, 2024
@mirrexagon mirrexagon merged commit 112c9d1 into mirrexagon:master Aug 26, 2024
@cortex
Copy link
Contributor Author

cortex commented Aug 26, 2024

Great, thanks for removing the git hacking and getting this merged! I'll take a look at the version stuff separately.

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.

7 participants