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

Allow user to manually choose sources directory during conversion #1897

Closed
rmartin16 opened this issue Jul 7, 2024 · 3 comments
Closed

Allow user to manually choose sources directory during conversion #1897

rmartin16 opened this issue Jul 7, 2024 · 3 comments
Labels
enhancement New features, or improvements to existing features. good first issue Is this your first time contributing? This could be a good place to start!

Comments

@rmartin16
Copy link
Member

What is the problem or limitation you are having?

The briefcase convert command should allow the user to choose a sources directory if a suitable one cannot be found instead of raising. For instance, if my project was named "My Favorite App", Briefcase is expecting a directory named "my_favorite_app"...but "myfavoriteapp" is reasonable and should be usable by the convert command.

Describe the solution you'd like

Allow users to choose an arbitrary sub-directory for the app sources.

Describe alternatives you've considered

Re-organize the existing project prior to conversion.

Additional context

No response

@rmartin16 rmartin16 added the enhancement New features, or improvements to existing features. label Jul 7, 2024
@freakboy3742
Copy link
Member

Agreed that it shouldn't be raising an error... that's an oversight of review on my part. The rest of the infrastructure is there to prompt the user - but we're raising an error before we get to that logic. The only complication I can see is that it's not obvious what the default value should be - but maybe a blank string will work (because it will be rejected by validation).

@freakboy3742 freakboy3742 added the good first issue Is this your first time contributing? This could be a good place to start! label Jul 11, 2024
@logan-keede
Copy link
Contributor

I was looking into this problem to make my first contribution since this was marked as "good first issue",
Here are the results of my code investigation:-
the error is raised during briefcase convert because that is more preferable to error raised in briefcase dev
The error in briefcase dev happens because of two reasons stated below:-

  1. AppConfig class which is used by briefcase dev command enforces self.module_name to be same as app name but hyphens are replaced by underscores. Since snake case is the persistent throughout the repository that is an understandable decision. config.py#356
  2. the main_module is defined as self.module_name in AppConfig and it must be present in source_list in pyproject.toml.
    config.py#417
    config.py#340
    (it seems these checks are present to establish a predictable structure)

However, setting an arbitrary main_module during convert command contradicts these checks and raises an error. so, the person who programmed that portion of convert command must have made the decision to enforce these checks.
commit 14e5c51

Since, all apps rely on AppConfig directly changing that just to accommodate arbitrary source directory might be detrimental to overall structure. This check enforces homogeneous structure for new briefcases and converted briefcases.

Approach to solutions

  1. Leave it as it is (maybe add instructions to ease the converting proccess)
  2. Change AppConfig to set first element in source_list to be main_module instead of module_name or make a separate element for that in pyproject.toml file

I am new to this codebase, so please correct me if I am missing something due to my limited understanding.
If 2nd approach is preferable, I believe I can implement that so consider assigning this issue to me.

Thanks for your patience,
logan

@freakboy3742
Copy link
Member

I was looking into this problem to make my first contribution since this was marked as "good first issue",
...
I am new to this codebase, so please correct me if I am missing something due to my limited understanding. If 2nd approach is preferable, I believe I can implement that so consider assigning this issue to me.

Thanks for taking a look at this! FWIW - we don't formally assign tickets; if the ticket is open, and nobody else appears to be working on it (i.e., there hasn't been a update for a couple of weeks), it's available.

As for your analysis of the ticket - on closer inspection, it appears my triage analysis was incorrect.

There's 3 names in use here:

  • the formal name of the app (e.g., "My App");
  • the app name - this is the name of module as declared in PEP 621 app metadata. It is likely similar to the formal name, but not required to be. Hyphens are legal, spaces are not, and some unicode normalisation is required. The name must be PEP 508 compliant; and
  • the module name - this is the Python module name used to start the app. This must be a valid Python symbol name, so some normalisation is performed from the PEP 508 app name - hyphens will be converted to underscores.

The original ticket description suggested that the last point (that the module name is a normalised form of the app name) isn't required. My analysis comment reinforced that idea. However, that assertion is incorrect - the module name must be derivable from the app name, as this a core part of how Briefcase starts an app.

This confusion occurs for 2 reasons.

Firstly, the original ticket refers to "project name" and "directory name", rather than the names used in the source code; and second, the source code refers to "app name" and "module name", and in my triage analysis, I got "formal name" and "app name", mixed up.

Secondly, in the new command, the app name is derived from the formal name (essentially replacing spaces with hypens); but the app name can be modified by the user if necessary. However, in the convert command, the candidate app name comes from PEP 621 app metadata, but can then be overridden by the user. The formal name is then gathered independently.

So - there's no requirement that the formal name be "normalisable" to the app name - but the module name must be normalisable from the app name, and the directory name must match the module name.

Using the example from the original ticket - a "project" named "My Favorite App", with a directory named my_favorite_app or myfavoriteapp - both are legal, but both are currently possible. The catch is that if the directory is named my_favorite_app, then the app name must be either my-favorite-app or my_favorite_app (or, I guess my-favorite_app or my_favorite-app...); but if the directory name is myfavoriteapp, then the app name must be myfavoriteapp as well.

To that end, the validation that is being performed is correct. It will raise an error if it can't find .../module_name/__main__.py in the code - and that's the actual requirement at runtime. I can't think of any legal directory structure that wouldn't be caught by the code in get_source_dir_hint() (although the hint might pick the wrong directory if there are multiple matches() - which is why it's a hint, not a hard-coded value).

So - thanks for your interest in this ticket - but it appears I've sent you on a wild goose chase. I'm going to close this ticket as invalid. Apologies for the confusion and wasted effort; hopefully you found the triage process worthwhile, and you'll be able to use that knowledge on another "first timer" ticket. #1900 is one possibility that sticks in the convert command; #1669 might be another candidate - there's a draft PR for that one, but it hasn't been looked at for 7 months.

@freakboy3742 freakboy3742 closed this as not planned Won't fix, can't repro, duplicate, stale Dec 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New features, or improvements to existing features. good first issue Is this your first time contributing? This could be a good place to start!
Projects
None yet
Development

No branches or pull requests

3 participants