-
Notifications
You must be signed in to change notification settings - Fork 131
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
[Documentation] Update instructions to build SWT binaries locally #1174
[Documentation] Update instructions to build SWT binaries locally #1174
Conversation
cc @akurtakov , @HannesWell |
Test Results 298 files - 1 298 suites - 1 1h 8m 30s ⏱️ + 1h 2m 17s For more details on these errors, see this check. Results for commit 74bda8a. ± Comparison against base commit 979d3f1. ♻️ This comment has been updated with latest results. |
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.
Thank you @fedejeanne for this.
I have added some comments below how this could be further enhanced.
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.
Thanks for the tips, @HannesWell , I changed some stuff based on them, WDYT?
I can squash all my commits before merging.
b360c59
to
058a706
Compare
058a706
to
96e475f
Compare
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.
I just had another pass over the documentation and since there were many small points to further enhance (mainly in the already existing doc, but it IMHO fits into this change) I just applied the suggestions by myself and pushed them in a separate commit.
Please incorporate them as you see fit.
A short overview is
- Removed outdated information about Hudson builds.
- Remove links and mentioning of specific JDK vendors. From my impression there is not only one source for JDK binaries anymore and I assume that everyone that wants to contribute to SWT has already set up a JDK anyways.
If we want to provide a link, I suggest to use a neutral place like:
https://adoptium.net/marketplace/?version=17 - There is no help for Windows and Mac so I removed its mentioning.
Thank you for these updates. They are definitely due.
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.
If I am not mistaken, the descriptions how to build the binaries by hand (directly executing the build.sh
/ build.bat
scripts), do not work as described. I would propose to completely remove these descriptions, as there is a Maven configuration that does everything required and replication all the information in a documentation leads to unnecessary duplications that needs to be kept consistent.
Alternatively run the `build.sh` script in `binaries/org.eclipse.swt.gtk.linux.${arch}/bin/library` directory. To | ||
use the locally build natives run the build script with the 'install' argument | ||
(`./build.sh install`) or --help to see more options. | ||
See **Building and Testing locally** in [Readme.md](Readme.md) |
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.
In the requirements, I would update the reference to GTK:
gtk3-devel
is the Fedora package name, on Ubuntu/Debian it islibgtk-3-dev
- We could/should mention GTK 4 as well
- We could simply refer to the GTK project installation page for the required (an always up-to-date) information: https://www.gtk.org/docs/installations/linux
Alternatively run the `build.bat` script under `org.eclipse.swt\Eclipse SWT PI\win32\library\` directory. | ||
To use the locally build natives run the script with the 'install' and `clean` arguments (`.\build.bat clean install`). |
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.
Does not work for the same reasons as described for Linux (except the OUTPUT_DIR
problem).
Just as a quick-reply for now: |
@HannesWell , @HeikoKlare , thank you for the comments, I addressed almost all of them in d556545 and made some other minor changes. I am also in favor of simplifying these instructions as much as possible and let the "hackers" figure out the alternative ways (in case they don't want to run Maven) on their own. I'm positive that the current state of these instructions is good enough to compile the binaries in the majority of the setups. Once this PR is good to go, I will squash the commits. @HannesWell We can add a permanent link to the desired commit after merging this PR if you want to. Just to be sure: you want to link this line (but not in master), right? eclipse.platform.swt/binaries/pom.xml Line 93 in 979d3f1
How would you phrase it in the documentation (in the follow-up PR)? |
The failing tests are unrelated |
Sorry, but inverse changes are actually needed. You are removing steps for manual building, but that's exactly what I was always doing, and I was pretty much committer number one for a few years. I was running steps manually because I'm not using Eclipse for developing SWT. And when I run maven from IDEA, it downloads unneeded stuff (tycho etc) for some 20 minutes before actually building. |
There seems to be some confusion here, as the proposed change removes nothing but redundant and outdated documentation. You can still do a "manual build" (i.e., do by hand what Maven does automatically). Having an explanation of that process in the documentation would be nothing else than writing down the according Maven step in natural language: eclipse.platform.swt/binaries/pom.xml Lines 92 to 134 in 979d3f1
So as @HannesWell proposed (#1174 (comment)), adding a link to the Maven configuration in the documentation for those who want to perform the steps by hand would be good. |
I still think that instead of removing documentation, it should be fixed, so removing is half step in wrong direction. It is not redundant, because parsing maven files is hard for people who never dealt with maven before (such as me). I'm merely commenting and do not intend to hold this PR. |
I think the point is that while the actual logic as spelled out in the pom.xml may evolve in the future (logic that is well tested by the builds and by the other developers) the documentation will forever need to evolve manually in lockstep. That's likely to be forgotten or not to happen. I image that most people will be happy if they can invoke Maven without further thought about the details, and even if that takes 20 minutes the first time to fill the cache, that's probably for most people better tradeoff than trying to read and to follow more detailed documentation instructions. So I do understand your concerns, I think this is a little like with Oomph where it's must easier to maintain something that's regularly and automatically used, than it is to maintain documentation that's rarely read, rarely used, and error prone to follow exactly to the letter... |
@SyntevoAlex I agree with @HannesWell , @HeikoKlare and @merks here: pointing to the Maven goal that does the trick (in a follow-up PR) is enough here so I wouldn't hold this PR just to add the "manual" steps which are likely to be outdated in the near future anyway. So as soon as @HeikoKlare and @HannesWell give me the green light (I hope I addressed every one of your comments), I'm squashing + merging :-) |
I understand your point that it is harder to parse Maven files than reading documentation that explains the Maven files in comparably simple language (even though that does not change anything w.r.t. it being redundant). However, maintaining the documentation is even harder, as only very few people will need that documentation (as stated by @merks). In my opinion, it even leads to the opposite: the people that may be interested in compiling the natives will run across all the information on how to execute the build.sh/build.bat and be tempted to follow that approach instead of simply executing the Maven build. So the documentation would have to be structured very well to lead the people in the proper direction, i.e., to just use the simple, Maven-based solution and understand that all the information about the manual build is irrelevant for them. |
d556545
to
74bda8a
Compare
Addressed #1174 (comment) and also added the perma-link from #1174 (comment) in 74bda8a |
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.
Looks good to me. Thank you for updating and cleaning up the documentation!
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.
Hoping that it will be a good compromise I made more suggestions for improvements below.
bundles/org.eclipse.swt/Readme.md
Outdated
For more details see the co-located platform specific Readme files: | ||
- [Readme Linux](Readme.Linux.md) | ||
- [Readme macOS](Readme.macOS.md) | ||
- [Readme Windows](Readme.Win32.md) |
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.
For more details see the co-located platform specific Readme files: | |
- [Readme Linux](Readme.Linux.md) | |
- [Readme macOS](Readme.macOS.md) | |
- [Readme Windows](Readme.Win32.md) |
This does not provide details anymore.
- Move the part about building and testing to the general Readme file since it can now be done in the same way in Windows, Mac and Linux. - Adapt the part about configuring the workspace to run snippets in Windows. - Move the "Development overview" to the general Readme file - Move the part about running the snippets to the Readme.md and change it so it mentions the pre-configured IDE installation (it's way easier) - Also add the perma-link to the section of the POM that calls Ant - Mention GTK4 in Linux and link to official website - Provide links to Adoptium's JDK 17 in all three readmes - Remove the part about setting SWT_JAVA_HOME in all 3 platforms Co-authored-by: Hannes Wellmann <wellmann.hannes1@gmx.net>
938e16d
to
b609f92
Compare
Sealed and delivered. Thank you all for participating! |
Contributes to #1171
See #1167 (comment)
Move some sections of the documentation to the general
Readme.md
file and change some outdated/unclear documentation in Windows.FYI I provided 2 commits for clarity but I will squash them before merging.