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: warn user about deprecated LV_DEFAULT_DRIVE_LETTER #7620

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

Conversation

AndreCostaaa
Copy link
Contributor

@AndreCostaaa AndreCostaaa commented Jan 16, 2025

Fixes #7614

Like explained here: #7614 (comment)

The line #define LV_FS_DEFAULT_DRIVE_LETTER LV_FS_DEFAULT_DRIVER_LETTER is causing the docs generation workflow to crash when building the examples because of a symbol redefinition. This checks if the symbol is already defined to avoid redefining it

uLipe
uLipe previously approved these changes Jan 17, 2025
@XuNeo
Copy link
Collaborator

XuNeo commented Jan 23, 2025

Maybe it's better to fix /home/runner/work/lvgl/lvgl/emscripten_builder/lv_conf.h:793:9 to use correct macro?

@AndreCostaaa
Copy link
Contributor Author

AndreCostaaa commented Jan 23, 2025

Maybe it's better to fix /home/runner/work/lvgl/lvgl/emscripten_builder/lv_conf.h:793:9 to use correct macro?

So that's where the config comes from ! To be honest i should've checked the build script before but i guess it's never too late

But it seems like a nightmare to maintain a config in another repository that can easily break. Would moving the config to this repository and copying it during the build process be a viable option?

@liamHowatt
Copy link
Member

There are three reasons the docs build is failing. I fixed it in this PR: lvgl/lv_web_emscripten#26 🙂

This is an important PR in general, I think.

@AndreCostaaa
Copy link
Contributor Author

Nice one! We can close this one then

@AndreCostaaa
Copy link
Contributor Author

This is an important PR in general, I think.

I don't follow. In what sense is it important?

@liamHowatt
Copy link
Member

When that config got renamed, there were two mechanisms added both trying to solve the same problem. This line in the API map, and the below.

/* Renamed config backwards-compatibility */
#if !defined(LV_FS_DEFAULT_DRIVER_LETTER) && defined(LV_FS_DEFAULT_DRIVE_LETTER)
#define LV_FS_DEFAULT_DRIVER_LETTER LV_FS_DEFAULT_DRIVE_LETTER
#endif

I don't feel great about either. I think maybe the one in lv_conf_internal_gen.py could go away, and the one in lv_api_map_v9_1.h could become

#ifdef LV_FS_DEFAULT_DRIVE_LETTER
#error deprecate config name. Rename to LV_FS_DEFAULT_DRIVER_LETTER
#endif

but that's just a 30 second idea. Open to any ideas whatsoever.

@AndreCostaaa
Copy link
Contributor Author

AndreCostaaa commented Jan 23, 2025

Thank you for clarifying. I understand now

On one hand I understand the idea of keeping backwards compatibility. On the other hand, the fact that the internal generation script got changed to add this feels a bit hacky imo

Also, I think that the user should at least get a warning if they're using a deprecated config. What do you think of removing this :

/* Renamed config backwards-compatibility */
#if !defined(LV_FS_DEFAULT_DRIVER_LETTER) && defined(LV_FS_DEFAULT_DRIVE_LETTER)
#define LV_FS_DEFAULT_DRIVER_LETTER LV_FS_DEFAULT_DRIVE_LETTER
#endif

And redefining it while providing a #warning inside lv_api_map_v9_1.h instead ?
Something like:

#ifdef LV_FS_DEFAULT_DRIVE_LETTER
    #define LV_FS_DEFAULT_DRIVER_LETTER LV_FS_DEFAULT_DRIVE_LETTER
    #warning deprecate config name. Rename to LV_FS_DEFAULT_DRIVER_LETTER
#endif

@AndreCostaaa AndreCostaaa reopened this Jan 23, 2025
@AndreCostaaa AndreCostaaa changed the title fix: check if the symbol is defined before redefining it fix: warn user about the depreacted LV_DEFAULT_DRIVE_LETTER Jan 23, 2025
@AndreCostaaa AndreCostaaa changed the title fix: warn user about the depreacted LV_DEFAULT_DRIVE_LETTER fix: warn user about deprecated LV_DEFAULT_DRIVE_LETTER Jan 23, 2025
Copy link
Member

@liamHowatt liamHowatt 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 doing this.

src/lv_api_map_v9_1.h Outdated Show resolved Hide resolved
Copy link
Member

@liamHowatt liamHowatt left a comment

Choose a reason for hiding this comment

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

I think this must be close to the best solution. Thanks. I think that '\0' check is a really good idea.

src/lv_api_map_v9_1.h Show resolved Hide resolved
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.

LVGL Docs deployment failed
4 participants