feat: allow for specifying ATs for upstream sources#377
Open
Toffikk wants to merge 10 commits intoPaperMC:mainfrom
Open
feat: allow for specifying ATs for upstream sources#377Toffikk wants to merge 10 commits intoPaperMC:mainfrom
Toffikk wants to merge 10 commits intoPaperMC:mainfrom
Conversation
These changes provide fork developers with a way to specify an AT file (or have it detected automatically) for upstream sources, such as `paper-server` and `paper-api` for a fork of Paper. This allows fork developers to benefit from using ATs while modifying these sources without needing to clutter patches, with lots of manual access transforms, and as such this greatly improves the overall comfort of making more involving changes, in the sources. It also allows for fork devs to be able to organize ATs in one place which makes it easier to review later. The implementation proposed in this commit works as follows: 1. Derive the location of the build-data dir from the output dir specified in the PatchRepo/Directory's config. 2. Search for an AT file, by the name of the patch repo/directory as specified. 3. Apply them before applying source patches. The locations in steps 1 and 2 can be overriden per patch repo/directory by just overriding the val. In order to achieve this change, we introduce a new task, with its implementation sitting in SetupForkUpstreamSources which applies those ATs. Everything else not explained here or even explained should follow the behaviour of AT application for minecraft sources. The change has been tested in production in an organisation fork of paperweight for over 6 months with both a nested fork project that uses them heavily and then fork projects depending on the nested fork project, and no issues have been reported or arose, so i'm confident that this implementation will work properly.
342d27d to
3665104
Compare
Contributor
Author
|
note: if #376 gets merged before this, this pr needs to be updated, |
8cc10ea to
55d76c0
Compare
310f23a to
4f1edf1
Compare
9f01971 to
617e64b
Compare
This is needed for being fully able to represent types in API projects as they are resolved from the root project and not nested, meaning the compile classpath passed is null as its from the root project.
fixes more binary representation issues
…minecraft ATs This fixes some issues with JST and forks as there are custom types from the API imported by paper, which arent contained in the mache libraries config and that causes JST to fail to create binary representations for some methods/classes thus not allowing to AT them, this fixes it
951a35a to
09cce04
Compare
…ath for minecraft ATs" This reverts commit 09cce04. seems to fail the tests for some reason, a dev can take a look at this, although the output of the log file is correct locally
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
These changes provide fork developers with a way to specify an AT file (or have it detected automatically) for upstream sources, such as
paper-serverandpaper-apifor a fork of Paper. This allows fork developers to benefit from using ATs while modifying these sources without needing to clutter patches, with lots of manual access transforms, and as such this greatly improves the overall comfort of making more involving changes, in the sources. It also allows for fork devs to be able to organize ATs in one place which makes it easier to review later.The implementation proposed in this commit works as follows:
Projectinstance orLayoutby injecting, however we cannot do it this way, as the upstream server projects are set up in the fork’s server project, not as part of the upstream setup like API is, meaning they either can’t access their ATs or other issues appear. Thelayoutapproach has even more downsides as it inherits the downsides of the aforementioned approach and thebuild-datadir would be forced to appear in the server project.outputDirto calculate the possible root project is the best way to make it work, as it works on fork setups and doesn’t cause issues, and we can also safely assume that it will work for everyone as every fork and fork template is following the convention of specifying theiroutputDir’s in a subfolder of the root project, and regardless it can be overrode like the minecraft AT file currently can be so this is correct in practice.The locations in steps 1 and 2 can be overriden per patch repo/directory by just overriding the val.
In order to achieve this change, we introduce a new task, with its implementation sitting in SetupForkUpstreamSources which applies those ATs. Everything else not explained here or even explained should follow the behaviour of AT application for minecraft sources.
The change has been tested in production in an organisation fork of paperweight for over 6 months with both a nested fork project that uses them heavily and then fork projects depending on the nested fork project, and no issues have been reported or arose, so i'm confident that this implementation will work properly.
For additional reference: