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

Add option to specify which repositories to generate SConscript files for #1192

Merged

Conversation

calebchalmers
Copy link
Contributor

Currently, the modm:scons:build module will generate an SConscript for itself and every other repository included in the build process. This causes issues if another repository has its own method of generating an SConscript, since modm will overwrite it.

This PR changes the behaviour to only generate SConscript files for modm and any additional repos specified in modm:scons:build:include_repos (comma-separated list of repository names).

Note: this is a minor breaking change since no additional repositories will be included by default, whereas previously they would have been.

@calebchalmers
Copy link
Contributor Author

Tests are failing due to:

Traceback (most recent call last):
  File "/__w/modm/modm/repo.lb", line 55, in <module>
    import modm_devices.parser
ModuleNotFoundError: No module named 'modm_devices'

Any ideas what the issue is here?

ext/mpaland/printf Outdated Show resolved Hide resolved
@salkinium
Copy link
Member

Good idea, but I don't like this implementation as it's not backward compatible and the option is not user friendly (although it could be defaulted I guess).

I'd rather you check the buildlog if any of the repos already generated a SConscript file in their output path, and then not generate ours (but still call it of course).

@calebchalmers
Copy link
Contributor Author

I'd rather you check the buildlog if any of the repos already generated a SConscript file in their output path, and then not generate ours (but still call it of course).

Yeah I was trying to think of the best way to go about this. I like your idea to check for existing files, but would we need to worry about build order?

What if another repo generates its SConscript in post_build?

@salkinium
Copy link
Member

We need a post_post_build step!1!! Lol no, there's an undocumented module.order : int = 0 that you can set in the init() to enforce module order. Here we could set it to 10000 to enforce it's build after all other post_build steps.

Copy link
Member

@salkinium salkinium left a comment

Choose a reason for hiding this comment

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

Could you check if my simplification still works correctly for your use case?

Copy link
Member

@rleh rleh left a comment

Choose a reason for hiding this comment

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

Nice 👍🏽

Thank you a lot!

@calebchalmers
Copy link
Contributor Author

Could you check if my simplification still works correctly for your use case?

Yep, works for me 👍

@salkinium salkinium merged commit fd8d79e into modm-io:develop Jul 21, 2024
12 checks passed
@calebchalmers calebchalmers deleted the feature/whitelist-scons-repos branch July 21, 2024 09:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants