-
Notifications
You must be signed in to change notification settings - Fork 101
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
Update plugin(s) build setup for distribution #1033
Conversation
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thelovekesh For building the standalone plugins, this looks solid so far.
However, I don't think it makes sense to introduce a build process for Performance Lab, this is a solution in search of a problem IMO. And I don't see a good reason to use .distignore
over .gitattributes
, please clarify why you're proposing this change.
873a924
to
1d82457
Compare
}, | ||
"scripts": { | ||
"changelog": "./bin/plugin/cli.js changelog", | ||
"since": "./bin/plugin/cli.js since", | ||
"readme": "./bin/plugin/cli.js readme", | ||
"translations": "./bin/plugin/cli.js translations", | ||
"build-plugins": "./bin/plugin/cli.js build-plugins", | ||
"build-plugins": "npm-run-all 'build:plugin:!(performance-lab)'", | ||
"build:plugin:performance-lab": "rm -rf build/performance-lab && mkdir -p build/performance-lab && git archive HEAD | tar -x -C build/performance-lab", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@felixarntz Added this command, in case we ever need to confirm what's being added in the build by deploy action.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@felixarntz I believe that we should use the above command to verify distribution files(maybe just before a release?). I can see that the build-cs
directory is being committed in the svn at https://plugins.trac.wordpress.org/browser/performance-lab/trunk for the most current release 😞.
Also, it would be good if we log an issue to remove such redundant files/dirs from all our plugins in the next release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thelovekesh This looks quite good to me now in terms of the overall approach. A few small comments/questions below.
I'm not very familiar with GitHub workflows, so it would be great to get @swissspidy's feedback on those changes in particular.
- name: Generate checksum | ||
working-directory: ./build/dist | ||
run: | | ||
mkdir -p $RUNNER_TEMP/plugin-checksums | ||
find . -type f -print0 | sort -z | xargs -r0 shasum -a 256 -b | sed 's# \*\./# *#' > $RUNNER_TEMP/plugin-checksums/checksums.txt | ||
shasum -a 256 -U -c $RUNNER_TEMP/plugin-checksums/checksums.txt | ||
cat $RUNNER_TEMP/plugin-checksums/checksums.txt | while read sum file; do echo "$sum $file" > ${file#\*}.sha256; done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need to create checksums? A bit overkill IMO. I doubt anyone would ever look at these.
If someone wants checksum verification, WordPress.org already provides that information anyway through a nice API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not a "must use," but providing a means to verify the integrity of your distributed assets is never bad.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good 👍 Looking forward to seeing this in action.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thelovekesh, thank you for the updates.
It seems that npm run test-plugins
is broken. Have you checked the automated and manual plugin release workflow with dry-run
set to true? This way, we can thoroughly test all steps of the workflow. Could you please ensure the following:
- Added a pull_request for checking the automated workflow.
- Set both auto and manual workflow DRY Run to true for security.
- Updated SVN credentials for double security.
Thanks @mukeshpanchal27. It seems like breaking changes are unrelated to this PR. I can see them on #1028 as well - https://github.com/WordPress/performance/actions/runs/8182298569/job/22373438106
Yes, I have tested them on my fork.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thelovekesh @swissspidy How does this PR determine which plugins to deploy an update for? Here's what I mean:
- When a new Performance Lab release is triggered for instance, not all standalone plugins will need to be updated.
- A plugin should only be updated if its new version is greater than the latest version currently available on wordpress.org.
I'm not certain the workflow currently accounts for that. I know the 10up deploy action handles this internally already, but I don't see anything in the other steps of our workflow that take that into account. Can you double check? 🤔
Also, another somewhat related comment on which ZIP files get attached to which GitHub release. FWIW, if that complicates getting this merged, we could always leave that out and add separately, also to solve it holistically for Performance Lab as well. We need to rethink our release strategy anyway.
@felixarntz At this point, no it doesn't take that into account and it's that way only. We need to re-consider on how we release our plugins and IMO that should be a part of different issue/PR. |
cfd474d
to
beb0500
Compare
I agree with you regarding the attachment of ZIP files to the release that it can be solved separately. However, the part about detecting whether a plugin even needs to be updated has to be handled here, otherwise it breaks existing behavior. For example:
In other words, this would be incorrect since the 1.0.5 ZIP generated wouldn't actually be the 1.0.5 release. That's why this needs to be fixed here to prevent this bug. |
This reverts commit 65e4273.
beb0500
to
0d5451d
Compare
sure @felixarntz. I have updated the workflow only to deploy a plugin if it's not already deployed on the WPOrg. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thelovekesh Looks great to me!
Before merge, @swissspidy could you take another brief look at the new additions since you had previously approved?
- name: Upload release assets | ||
if: steps.artifact-existence.outputs.exists == 'true' | ||
uses: softprops/action-gh-release@v1 | ||
with: | ||
files: | | ||
./build/dist/${{ matrix.plugin }}.zip | ||
./build/dist/${{ matrix.plugin }}.zip.sha256 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this will attach all standalone plugin ZIPs to the Performance Lab plugin release? If so, it seems like a missing piece in general for the standalone plugin story is how to trigger a release of a standalone plugin without having to do a release of Performance Lab as well. I guess that can still be done by manually invoking the workflow, but then there would be no record of the standalone plugin release among the GitHub releases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's right. For now, there are two scenarios:
- Release trigger.
- On workflow dispatch.
In the second case, there will be no release record as mentioned by @westonruter. In the last Performance chat, we discussed opening a new issue which I have just opened #1061 to discuss it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@westonruter Correct, at the moment the manual workflow is the way to do it, if we need to release something apart from a PL release.
A discussion on how to better address that is long overdue and has come up in a few places before. Let's tackle that in a separate issue. Potentially we can just create GitHub releases for the individual plugins and have the workflows adjust based on e.g. the release name or something like that what gets deployed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this will attach all standalone plugin ZIPs to the Performance Lab plugin release?
Only for such plugins that are not already released on WPOrg. See 0d5451d
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still looks good to me now. I'd say let's just get this in and give it a try, we can always refine later if needed.
Summary
Fixes task 3 of #1012
As mentioned by @westonruter in #893 (comment), we are now moving towards use cases where we can have to build files that are not in the git tree and hence will be ignored by
git archive
.Also as outlined in #1024 (comment), in the new mono repo setup we will require a build setup because the repo level
.gitattributes
or.distignore
won't work just to ignore files from standalone distributed plugins.This PR proposes a simple Webpack-based plugin bundling solution that solves both problems.
Relevant technical choices
copy-webpack-plugin
Webpack plugin.readme.txt
file. This task is now handled by Webpack which creates amanifest.json
with plugin versions.Screenshot
Screenshot
Checklist
[Focus]
orInfrastructure
label.[Type]
label.no milestone
label.