Skip to content
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

8231870: CrossLibs script for armv6hf toolchain download fails #8

Conversation

@dellgreen
Copy link
Contributor

dellgreen commented Oct 4, 2019

buildSrc/crosslibs-armv6hf.sh pulls down debian and raspbian packages to be able to cross compile javafx for arm hard float. Up to now the upstream distribution versions have been debian and raspbian wheezy, but these are now end of life and have been archived to servers that have different domain names.

Tried to change to use jessie but this generates a whole load of __THROWNL errors, so for now have updated the domain names to point to new servers so that wheezy packages can still be retrieved and cross compilation succeeds.

https://bugs.java.com/bugdatabase/view_bug.do?bug_id=JDK-8231870

Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

JDK-8231870: CrossLibs script for armv6hf toolchain download fails

Approvers

  • Johan Vos (jvos - Reviewer)
@dellgreen dellgreen changed the title JDK-8231870: Updated armv6hf crosslibs script with domains JDK-8231870: Updated armv6hf crosslibs script with new domains Oct 4, 2019
@bridgekeeper bridgekeeper bot added the oca label Oct 4, 2019
@bridgekeeper
Copy link

bridgekeeper bot commented Oct 4, 2019

Hi dellgreen, welcome to this OpenJDK project and thanks for contributing!

We do not recognize you as Contributor and need to ensure you have signed the Oracle Contributor Agreement (OCA). If you have not signed the OCA, please follow the instructions. Please fill in your GitHub username in the "Username" field of the application. Once you have signed the OCA, please let us know by writing /signed in a comment in this pull request.

If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please use "Add GitHub user dellgreen" as summary for the issue.

If you are contributing this work on behalf of your employer and your employer has signed the OCA, please let us know by writing /covered in a comment in this pull request.

@dellgreen
Copy link
Contributor Author

dellgreen commented Oct 4, 2019

/signed
/covered

@dellgreen
Copy link
Contributor Author

dellgreen commented Oct 4, 2019

@dellgreen
Copy link
Contributor Author

dellgreen commented Oct 4, 2019

https://www.oracle.com/technetwork/community/oca-486395.html#i

Ideaworks Ltd. - OpenJDK OpenJFX - dellgreen

@dellgreen
Copy link
Contributor Author

dellgreen commented Oct 4, 2019

i don't know why jcheck is failing this, when a different pull-request i submitted yesterday works fine. They both look like they adhere to the guidelines?

@dellgreen dellgreen changed the title JDK-8231870: Updated armv6hf crosslibs script with new domains 8231870: Updated armv6hf crosslibs script with new domains Oct 4, 2019
…mes for wheezy

Updated crosslibs-armv6hf.sh with new toolchain location domain names as wheezy is end of life
@dellgreen dellgreen force-pushed the dellgreen:dpg/8231870/armCrossbuildToolchainDomainChange branch from f809aab to bb4bcc9 Oct 4, 2019
@bridgekeeper bridgekeeper bot removed the oca label Oct 7, 2019
@openjdk openjdk bot added the rfr label Oct 7, 2019
@mlbridge
Copy link

mlbridge bot commented Oct 7, 2019

Webrevs

@kevinrushforth
Copy link
Member

kevinrushforth commented Oct 7, 2019

For some reason the RFR email wasn't sent. The Skara team is looking into this and should have a solution soon. In the mean time, we can proceed with the review.

@kevinrushforth
Copy link
Member

kevinrushforth commented Oct 7, 2019

@johanvos can you review this?

Copy link
Member

kevinrushforth left a comment

I don't have any particular issue with this change, but I will defer the review to @johanvos.

It does highlight an existing problem where we are still using an http URL rather than https. That may or may not be something we can solve here.

@@ -228,7 +228,7 @@ installLibs() {

getPackages \
$DESTINATION \
http://ftp.us.debian.org/debian/ wheezy main armhf \
http://archive.debian.org/debian/ wheezy main armhf \

This comment has been minimized.

Copy link
@kevinrushforth

kevinrushforth Oct 7, 2019

Member

The use of http:// URLs to download artifacts is strongly discouraged, since it isn't secure. Is there a valid https:// URL that can be used instead? I note that just substituting http with https in the above URL does not work.

This comment has been minimized.

Copy link
@octylFractal

octylFractal Oct 7, 2019

In general a lot of Debian package URLs are not HTTPS by default because of apt's built-in signature checking, https://whydoesaptnotusehttps.com/. However, it does seem like it should at least be supported, so it might be a bug in the debian.org server config.

Additionally, I see that the getPackages command doesn't check these signatures. It probably should, but that's another PR.

@@ -389,7 +389,7 @@ installLibs() {
# get some rapberry Pi specials
getPackages \
$DESTINATION \
http://archive.raspbian.org/raspbian wheezy firmware armhf \
http://legacy.raspbian.org/raspbian wheezy firmware armhf \

This comment has been minimized.

Copy link
@kevinrushforth

kevinrushforth Oct 7, 2019

Member

Same comment hear about using an https URL if possible.

@dellgreen
Copy link
Contributor Author

dellgreen commented Oct 7, 2019

https for legacy.raspbian.org works
https://archive.kernel.org as a replacement for archive.debian.org looks promising.
I'm away on a business trip until Wednesday. I can test these domains with the script then

@johanvos
Copy link
Collaborator

johanvos commented Oct 8, 2019

I confirm that without the patch, the "cross-build tools" can not be fetched. With this patch, the tools can be fetched and the build can be created using these tools.
I think this PR is good as it is, as it fixes something that was broken.

However, in general I think the concept of this crosslibs script is broken for a number of reasons:

  1. for none of the other targets, we have a script for downloading the toolchain.
  2. we have one big blob toolchain for all ARM devices, and do not take advantage of new compilers/libraries/CPU's. Maintaining toolchains requires work, and I think it is better that the OpenJFX repository focuses on the source code, not on the build context.

Rethinking the concept of cross-compiliation involves much more than just downloading a cross-compiler and libs, and we should not fix that in a rush.

I therefore propose to merge this PR.

@bridgekeeper
Copy link

bridgekeeper bot commented Oct 8, 2019

You are already a known contributor!

@kevinrushforth
Copy link
Member

kevinrushforth commented Oct 8, 2019

I am going to temporarily change the title in an attempt to force the jcheck bot to run again. I'll change it back once done. Failing this, I will ask the Skara admins to rerun the check if possible. If this doesn't work, we will need to close this PR and have you open a new one.

@kevinrushforth kevinrushforth changed the title 8231870: Updated armv6hf crosslibs script with new domains WIP: 8231870: Updated armv6hf crosslibs script with new domains Oct 8, 2019
@openjdk openjdk bot removed the rfr label Oct 8, 2019
@kevinrushforth kevinrushforth changed the title WIP: 8231870: Updated armv6hf crosslibs script with new domains 8231870: Updated armv6hf crosslibs script with new domains Oct 8, 2019
@openjdk openjdk bot added the rfr label Oct 8, 2019
@kevinrushforth
Copy link
Member

kevinrushforth commented Oct 8, 2019

Looks like that worked and reran jcheck.

@kevinrushforth
Copy link
Member

kevinrushforth commented Oct 8, 2019

Please ignore the above comments. I added them to the wrong PR.

This PR was and still is ready for review.

Copy link
Collaborator

johanvos left a comment

I am ok with the PR as is. The decision on how to install build tools (e.g. via apt-get, via a zip download, over http, over https) is not part of the code repository, and can be specific to the distributor.
This PR fixes something that was broken (unavailable host) so +1 for this.

@openjdk openjdk bot removed the rfr label Oct 9, 2019
@openjdk
Copy link

openjdk bot commented Oct 9, 2019

@dellgreen This change can now be integrated. The commit message will be:

8231870: CrossLibs script for armv6hf toolchain download fails

Reviewed-by: jvos
  • If you would like to add a summary, use the /summary command.
  • To list additional contributors, use the /contributor command.

Since the source branch of this PR was last updated there has been 1 commit pushed to the master branch:

  • c6eb091: 8231854: Change Mercurial to git in various README files

Since there are no conflicts, your changes will automatically be rebased on top of the above commits when integrating. If you prefer to do this manually, please merge master into your branch first.

As you are not a known OpenJDK Author, an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@kevinrushforth, @johanvos) but any other Committer may sponsor as well.

  • To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).
@openjdk openjdk bot added the ready label Oct 9, 2019
@dellgreen
Copy link
Contributor Author

dellgreen commented Oct 9, 2019

I'll suspend testing of the https domains as previously mentioned.

@kevinrushforth
Copy link
Member

kevinrushforth commented Oct 9, 2019

@dellgreen once you are ready, go ahead and issue the /integrate command. Presuming that @johanvos will sponsor this, he can then issue the /sponsor command after double-checking the commit message, etc.

@dellgreen
Copy link
Contributor Author

dellgreen commented Oct 9, 2019

/integrate

@openjdk
Copy link

openjdk bot commented Oct 9, 2019

@dellgreen
Your change (at version bb4bcc9) is now ready to be sponsored by a Committer.

@openjdk openjdk bot added the sponsor label Oct 9, 2019
@johanvos
Copy link
Collaborator

johanvos commented Oct 9, 2019

While checking, I notice that this PR has a different title than the title of the corresponding bug in JBS: https://bugs.openjdk.java.net/browse/JDK-8231870.
@kevinrushforth can we simply edit the title of the PR to match the one in JBS?

@kevinrushforth
Copy link
Member

kevinrushforth commented Oct 9, 2019

Btw, my username is @kevinrushforth so you notified someone else on the above comment...

Yes, editing the title to match the JBS issue is a very good thing to do. Since you are sponsoring it, go ahead and do that. I don't know for certain whether the bot will pick it up (since the contributor has already done a /integrate) -- it will be a good test.

@johanvos johanvos changed the title 8231870: Updated armv6hf crosslibs script with new domains 8231870: CrossLibs script for armv6hf toolchain download fails Oct 9, 2019
@johanvos
Copy link
Collaborator

johanvos commented Oct 9, 2019

I don't see a way to edit the commit message, so I'll just sponsor it and see if the bot modifies the commit message with the new title or not.

@johanvos
Copy link
Collaborator

johanvos commented Oct 9, 2019

/sponsor

@openjdk openjdk bot closed this Oct 9, 2019
@openjdk openjdk bot added integrated and removed sponsor labels Oct 9, 2019
@openjdk
Copy link

openjdk bot commented Oct 9, 2019

@johanvos @dellgreen The following commits have been pushed to master since your change was applied:

  • c6eb091: 8231854: Change Mercurial to git in various README files

Your commit was automatically rebased without conflicts.

Pushed as commit 63fe66c.

@openjdk openjdk bot removed the ready label Oct 9, 2019
@kevinrushforth
Copy link
Member

kevinrushforth commented Oct 9, 2019

I'll just sponsor it and see if the bot modifies the commit message with the new title or not.

I see that the bot did update the commit message to match your change to the title. Nice.

@dellgreen dellgreen deleted the dellgreen:dpg/8231870/armCrossbuildToolchainDomainChange branch Nov 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.