Generating Dockerfile refers to wrong script name

[toreau]

If you do this:

mojo generate app MyApp::Something

The following script is created:

script/my_app_something

But if you generate a Dockerfile using this:

script/my_app_something generate dockerfile

The Dockerfile will refer to a wrongly named CMD script WORKDIR path, ref. my_app_something vs. my_app-something:

$ cat Dockerfile 
FROM perl
WORKDIR /opt/my_app-something
COPY . .
RUN cpanm --installdeps -n .
EXPOSE 3000
CMD ./script/my_app-something prefork

[tianon]

Hmm, that's strange; here's the code responsible for that value:

mojo/lib/Mojolicious/Command/Author/generate/dockerfile.pm

Lines 11 to 12 in 4093223

	my $name = $self->app->moniker;
	my $exe = $ENV{MOJO_EXE} ? path($ENV{MOJO_EXE})->to_rel($self->app->home)->to_string : "script/$name";

It's generated either from MOJO_EXE or $self->app->moniker, and I'm a little surprised by this. 😬

I guess it maybe needs to use class_to_file instead somehow, like mojo generate app does?
(I'm grasping at straws here 😇)

mojo/lib/Mojolicious/Command/Author/generate/app.pm

Line 13 in 4093223

my $name = class_to_file $class;

Or perhaps this is technically a bug in moniker?

[kathackeray]

The name my_app_something for MyApp::Something is ambiguous (it would be the same for My::AppSomething). Generating app names in the form my_app-something (as the dockerfile generation assumes) would seem to be the superior solution.

Is such a solution acceptable, or is the naming for app generation constrained by some need for backward compability?

[kathackeray]

The decamelize() function in Mojo::Util handles transforming to - and _ appropriately.

However, the class_to_file() function in Mojo::Util removes occurrences of :: before handing off to decamelize().

sub class_to_file {
  my $class = shift;
  $class =~ s/::|'//g;
  $class =~ s/([A-Z])([A-Z]*)/$1 . lc $2/ge;
  return decamelize($class);
}

This ensures that decamelize() never receives any instances of :: to convert into -, resulting in _ only. There may be a reason for this that I haven't anticipated; no comment in the code.

If the second line of the function was changed to $class =~ s/'//g; then decamelize() returns unambiguous naming, and existing tests still pass. To be clear, the result of the proposed change to line 99 is as follows.

# $class =~ s/::|'//g;
MyApp::Something: my_app_something
My::AppSomething: my_app_something

# $class =~ s/'//g;
MyApp::Something: my_app-something
My::AppSomething: my-app_something

Any ideas over why :: is currently removed, and so any objection to a pull request which seeks to:

• Make the above change to ensure :: is not removed before handing off to decamelize()
• Ensure the same approach is used for dockerfile name generation
• Add more tests to Mojo::Util to cover this usage

[jberger]

An alternative might just be to not use a application-name-specific path inside the container. I see /app or the like used regularly.

[kathackeray]

That seems like a separate (and worthy) consideration to me. The Dockerfile copies the current directory into the container and then executes the script which mojo names by the given application name: CMD ./script/my_app-something prefork. In order to switch to generic naming as you suggest, mojo would need to generate its new apps with generic naming of the script (not just within the container), or else add excessive juggling to the Dockerfile. There are compelling reasons for considering the former.

However, even if it was decided to make the script name generic, it seems to me that the problem described above is still a problem, just one that was now hidden by removal of the known manifestation?

Do you agree based my previous post that, unless a reason arises why - is overlooked by class_to_file(), the function is currently forming ambiguous (and irreversible) file names?

[kathackeray]

That there is a reason seems probable, given that the tests for class_to_file encompass the current behaviour. (I wrongly stated earlier that the proposed change did not break existing tests).

  is class_to_file('FOOBAR'),    'foobar',    'right file';
  is class_to_file('FOO::Bar'),    'foobar',    'right file';

But without a definite reason (such as supporting a certain OS?) it perhaps ought to be changed. If a reason turns up, a comment alongside the code might prevent confusion cropping up again.

Edit: Aside from its tests, class_to_file is used only for naming the app script. If a generic name was adopted, it seems this function could be retired.

[jberger]

An alternative might just be to not use a application-name-specific path inside the container. I see /app or the like used regularly.

I missed that it isn't just the path but also the script name.

[kathackeray]

The first leg of my attempted fix for this in #1918 was rejected because it entails a breaking change. Given that any legitimate fix for this issue would be rejected on the same grounds, is this issue best closed?

[kraih]

The first leg of my attempted fix for this in #1918 was rejected because it entails a breaking change.

I don't see a rejection, you closed it after one unfavourable review. The real issue here is that we currently lack more reviewers in the project.

[kraih]

The name my_app_something for MyApp::Something is ambiguous (it would be the same for My::AppSomething). Generating app names in the form my_app-something (as the dockerfile generation assumes) would seem to be the superior solution.

Why does the script name have to be unique at all? To me it's much more important that it's easy to remember and type.

[kathackeray]

I'm not so much concerned with the name being unique, as with it being consistent. My PR for #1918 only addresses consistency: ensuring that class_to_file() is used exclusively for naming files during app generation. This happens to favour the simpler naming that you prefer. The naming of the app directory and script name is unchanged by the PR. The configuration file and dockerfile name are instead standardised with those for consistency.

Once #1918 was merged, it was my intention to verify that the dockerfile generation now works out-of-the-box and is correctly documented. This was the quickest and least controversial route to a fix I could plot.

But to respond briefly to your point about uniqueness, I believe there are benefits to the naming of the files (which is based on the class name) being non-ambiguous and reversible. Not least to accommodate automation. I also suspect most developers recall foremost the name of their class, My::Long::AppName for example, rather than the name of the script, and instinctively expect a file name derived from it to appear in a lossless way. Though I realise this is heavily subjective.

Making the script and configuration file names generic (app, config.yml) is likely the cleanest solution, which I'm happy to tackle if preferred. But if we're looking for the cheapest, least debatable path to fixing dockerfile generation, my PR might already be it.

[tianon]

Ooh, @kraih are you suggesting that having the app generator use script/app all the time instead might be acceptable? 👀

(IMO it still makes sense to try and fix the disparity between moniker and class_to_file, but that would at least resolve this particular facet of the issue)

[kraih]

Ooh, @kraih are you suggesting that having the app generator use script/app all the time instead might be acceptable? 👀

Probably not. But i don't expect them to be 100% unique for every module name variant, like my_app-something.

[kathackeray]

I'm happy to concede that point for now, if it helps push this PR through. As mentioned above, the PR #1918 does not alter how the script name is currently formed.