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 double project idf_ext.py loading (IDFGH-3287) #5278

Closed
wants to merge 1 commit into from

Conversation

BrianPugh
Copy link
Contributor

Fixes the issue described in #5193 . I did not combine the project extension loading with the generic built-in extension loading since it'd be slightly backward incompatible (but would have reduced ~10 lines of code).

@CLAassistant
Copy link

CLAassistant commented May 12, 2020

CLA assistant check
All committers have signed the CLA.

@github-actions github-actions bot changed the title Fix double project idf_ext.py loading Fix double project idf_ext.py loading (IDFGH-3287) May 12, 2020
@Alvin1Zhang
Copy link
Collaborator

@BrianPugh Thanks for your contribution.

@kumekay
Copy link
Collaborator

kumekay commented May 13, 2020

Hi @BrianPugh
Thank you for the PR!

I thought twice and realised that making path as a set is not a good idea,
because extensions from IDF_EXTRA_ACTIONS_PATH may depend on some built-in extensions, but in a set order is not determined, so it's better to keep it as list to keep the order:

    extension_dirs = [realpath(idf_py_extensions_path)]
    extra_paths = os.environ.get("IDF_EXTRA_ACTIONS_PATH")
    if extra_paths is not None:
        for path in extra_paths.split(';'):
            path = realpath(path) 
            if path not in extension_dirs:
                extension_dirs.append(path)

    extensions = {}

Could you please update the PR?

@BrianPugh
Copy link
Contributor Author

updated!

tools/idf.py Outdated
extra_paths = os.environ.get("IDF_EXTRA_ACTIONS_PATH", "").split(';')
extension_dirs = [idf_py_extensions_path] + extra_paths
extensions = {}
# Make a set to automatically handle duplicate paths
Copy link
Collaborator

Choose a reason for hiding this comment

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

This comment doesn't make sense anymore,
and could you remove it and squish all commits into one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed and squashed

@BrianPugh BrianPugh force-pushed the bug/double_idf_ext branch from 54e1e88 to 34a9238 Compare May 13, 2020 22:13
Copy link
Collaborator

@kumekay kumekay left a comment

Choose a reason for hiding this comment

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

Looks good, thank you

espressif-bot pushed a commit that referenced this pull request May 18, 2020
…d cwd if IDF_EXTRA_ACTIONS_PATH is not set.

Merges #5278
@BrianPugh
Copy link
Contributor Author

merged in 3a6b8bb

@BrianPugh BrianPugh closed this May 19, 2020
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