Skip to content

Merge webapp/vendor/Makefile into webapp/Makefile #2636

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

Merged
merged 2 commits into from
Aug 25, 2024

Conversation

tom93
Copy link
Contributor

@tom93 tom93 commented Jul 29, 2024

Continued from #2618 (comment).

tom93 added 2 commits July 29, 2024 11:10
I find it easier to follow when all the composer commands are in the
same Makefile.

Also move `composer auto-scripts` back into inplace-install-l, out of
concern for ordering issues. It was moved into a prereq of
inplace-install in commit ab267ec (Put lib/vendor close to the
webapp, 2024-06-23).
Just moving around the lines, no functional changes.
@tom93 tom93 mentioned this pull request Jul 29, 2024
@vmcj
Copy link
Member

vmcj commented Jul 30, 2024

I think this is personal preference as I find this harder to follow as I already stated in the other PR. Let's see what the rest thinks.

@vmcj vmcj requested a review from a team July 30, 2024 18:47
# the autoload file.
# We skip it if autoload_runtime.php already exists, to avoid running composer
# as root during `sudo make install-domserver`.
composer-dump-autoload: vendor/autoload_runtime.php
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if we need composer-dump-autoload again. The whole reason I started with the move was to get less phony targets so if possible I rather get rid of that 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.

I added it back for symmetry with composer-dump-autoload-dev, like composer-dependencies(-dev). But it's just a naming choice, happy to change it back.

@vmcj vmcj requested a review from eldering August 25, 2024 12:22
Copy link
Member

@eldering eldering left a comment

Choose a reason for hiding this comment

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

Looks OK to me.

@eldering eldering added this pull request to the merge queue Aug 25, 2024
Merged via the queue into DOMjudge:main with commit e78b7cc Aug 25, 2024
26 checks passed
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.

3 participants