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.

Sign up for GitHub

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

Jump to bottom

jetbrains.jdk: openjdk11 (11.0.13-b1751.25) → openjdk17 (17.0.3-b469.37) #183763

Merged
merged 3 commits into from
Sep 20, 2022
Merged

jetbrains.jdk: openjdk11 (11.0.13-b1751.25) → openjdk17 (17.0.3-b469.37) #183763

merged 3 commits into from
Sep 20, 2022

Conversation

fabianhjr
Copy link
Member

@fabianhjr fabianhjr commented Jul 29, 2022 •
edited

Description of changes

Two bundled updates, one of runtime (JBR) and IntelliJ IDEA Community Edition

I had been using the JBR 17 update for a while in previous versions of Idea Comumnity (only nuance were some warnings), with upstream moving the default forward this might be a good time to move nixpkgs default runtime forward too.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 22.11 Release Notes (or backporting 22.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@fabianhjr fabianhjr requested a review from edwtjo as a code owner July 29, 2022 18:36
@AnatolyPopov
Copy link
Member

Exactly the thing I was looking into today also! Will try to take a look soonish.

@fabianhjr
Copy link
Member Author

Just as a note, other JetBrains packages should be tested too since the runtime update affect those.

@AnatolyPopov
Copy link
Member

I was actually thinking about not dropping JBR11 but keeping both for sometime but will see

@drupol
Copy link
Contributor

drupol commented Aug 3, 2022 •
edited

Are you going to update all the other Jetbrains IDEs ? Thanks.

@fabianhjr
Copy link
Member Author

I was actually thinking about not dropping JBR11 but keeping both for sometime but will see

AFAIK all previous versions of editors (at least 2022.1) work great with JRE 17, there might have been some kinks/rough edges but JetBrains decided to move the default forward so no need to keep the previous versions.

@fabianhjr
Copy link
Member Author

Are you going to update all the other Jetbrains IDEs ? Thanks.

Not really, I don't use any other editor and this PR hasn't gotten reviews/merged as it is.

@AnatolyPopov
Copy link
Member

@fabianhjr I did take a look into these things and for some reason I still managed to build JBR17 locally.
It depends on openjdk17 that I believe is bootstrapped via openjdk17-bootstrap that is defined as

nixpkgs/pkgs/top-level/java-packages.nix

Line 126 in 511e56d

openjdk17-bootstrap = mkBootstrap adoptopenjdk-16

So when I'm trying to build JBR17 I'm getting an error with wrong bootstrap version complaining about openjdk16 and asking for JDK 11.

I also looked into the sources of JBR 17 and there required bootstrap versions are 17,18,19 AFAIR.
So I believe something goes wrong somewhere and seems the build for JBR17 needs to be changed.

Or can I you say what am I doing wrong?

@drupol
Copy link
Contributor

drupol commented Aug 4, 2022

Not really, I don't use any other editor and this PR hasn't gotten reviews/merged as it is.

Do you think you can update them? Other PRs related to jetbrains product are upgrading all the IDEs at once.

@fabianhjr
Copy link
Member Author

fabianhjr commented Aug 4, 2022 •
edited

@fabianhjr I did take a look into these things and for some reason I still managed to build JBR17 locally. It depends on openjdk17 that I believe is bootstrapped via openjdk17-bootstrap that is defined as

nixpkgs/pkgs/top-level/java-packages.nix

Line 126 in 511e56d

openjdk17-bootstrap = mkBootstrap adoptopenjdk-16

So when I'm trying to build JBR17 I'm getting an error with wrong bootstrap version complaining about openjdk16 and asking for JDK 11.

[..]

Or can I you say what am I doing wrong?

Seems like a darwin/macOS issue due to a typo, I believe it should be using version 17 to bootstrap. Checked the annotations and that line comes from this commit that doesn't mention using 16 to bootstrap 17 and probably went unnoticed until now.

a3401f6

@fabianhjr
Copy link
Member Author

Ah, nvm, seems like there is something going on with JDK 17 and darwin/macOS.

#143836 (comment)

I have no access to that platform, could you try to change that bootstrap version to 17 and check if it works? @AnatolyPopov

@fabianhjr
Copy link
Member Author

fabianhjr commented Aug 4, 2022 •
edited

Seems like adopt-openjdk 17 has been stalled on darwin/macOS since march?

#165354

Could you try to cherry-pick af9e23d for testing tho adopt-openjdk-17 for bootstrapping @AnatolyPopov?

/cc @NixOS/darwin-maintainers

@AnatolyPopov
Copy link
Member

@fabianhjr Thanks for digging into this but first of all I also do not have MacOS/Darwin, I'm on NixOS.
Second - I would suggest waiting for adoptopenjdk17 to be merged, there are several related PRs in progress.
Third - when I tried to build I tried with the JCEF version of JBR and there seems to be some issues #160468

I will try to run the build with JBR17 without JCEF a little later and we'll see how it will go.

@drupol
Copy link
Contributor

drupol commented Aug 5, 2022

I would also skip PHPStorm 2022.2 and use directly 2022.2.1 when it's out, see https://blog.jetbrains.com/phpstorm/2022/08/phpstorm-2022-2-1-preview/

@AnatolyPopov
Copy link
Member

@drupol I believe this PR will not include any updates except Intellij unfortunately.
Also, @fabianhjr since there are some complications with JBR17 could we maybe create separate PR for regular version upgrade and keep this one focused on JBR? Lates Intellij anyways seems to be working fine with JBR11

@fabianhjr
Copy link
Member Author

Done so, cherry-picked the "apdoptopenjdk: add 17.0.2" commit and change of bootstrap

@fabianhjr fabianhjr changed the title JetBrains Updates jetbrains.jdk: openjdk11 (11.0.13-b1751.25) → openjdk17 (17.0.3-b469.32) Aug 5, 2022
Mindavi
Mindavi approved these changes Aug 9, 2022
View reviewed changes
Copy link
Contributor

@Mindavi Mindavi left a comment

Choose a reason for hiding this comment

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

Diff LGTM, one question

@Mindavi
Copy link
Contributor

Mindavi commented Aug 9, 2022

There's a hash mismatch on Darwin too, see ofborg.

@fabianhjr
jetbrains.jdk: openjdk11 (11.0.13-b1751.25) → openjdk17 (17.0.3-b469.37)
27a4de4 
> With the IntelliJ IDEA 2022.2 EAP we are moving from JetBrains Runtime
  11 (JBR11) to JetBrains Runtime 17 (JBR17). Starting with this build,
  all IntelliJ IDEA 2022.2 updates will come with JBR17. This will have
  the following effects:
> - A significant performance improvement allowing faster and smoother
    IDE operation.
> - Better security, as JBR17 is based on the latest OpenJDK LTS.
> - Better rendering performance on macOS, as JetBrains Runtime 17
    leverages Metal API.
> - Increased accessibility on macOS, as JBR17 features integration with
    VoiceOver screen reader.
> - Usage of Vector API designed to express vector computations that
    compile at runtime to vector instructions on supported CPU
    architectures, thus achieving performance superior to equivalent
    scalar computations.

From: https://blog.jetbrains.com/idea/2022/05/intellij-idea-2022-2-eap-1/#JetBrains_Runtime
@fabianhjr
Copy link
Member Author

Rebased due to #165354 merge

@drupol
Copy link
Contributor

drupol commented Sep 19, 2022

Current Jetbrains tools are broken when it comes to the use SSH remote connection, this is a huge problem for me and I guess for some other people as well.

It would be nice to move on with this. WDYT ?

@veprbl veprbl self-assigned this Sep 19, 2022
@veprbl veprbl mentioned this pull request Sep 19, 2022
Closed
@veprbl veprbl removed their assignment Sep 19, 2022
@veprbl
Copy link
Member

veprbl commented Sep 19, 2022

Seems like fcfdb62 fixes the fetching issue. But the build of openjfx fails on x86_64-linux.

@fabianhjr
Copy link
Member Author

Current Jetbrains tools are broken when it comes to the use SSH remote connection, this is a huge problem for me and I guess for some other people as well.

It would be nice to move on with this. WDYT ?

A darwin contributor is needed to fix on darwin, on linux it works great.

@drupol
Copy link
Contributor

drupol commented Sep 19, 2022

Darwin maintainers cannot act as gatekeepers here. I propose to merge, and if there are issues on Darwin, then they should open a PR to fix it.

@winterqt
Copy link
Member

winterqt commented Sep 19, 2022 •
edited

Darwin maintainers cannot act as gatekeepers here. I propose to merge, and if there are issues on Darwin, then they should open a PR to fix it.

Who's saying they're... gatekeepers? Seems like a very odd word choice (that has weird implications with it).

If it'a a simple fix, there's no reason it can't be added to this PR. (Note: I have no clue if it is.)
And if it's not, then it's fine -- just make sure to mark it as broken on Darwin (ideally).

@uri-canva
Copy link
Contributor

Re: merging without darwin working, this is why we have the support tiers, see https://github.com/NixOS/rfcs/blob/master/rfcs/0046-platform-support-tiers.md.

Since this problem has been reported to @NixOS/darwin-maintainers in #183763 (comment), I believe there has been enough time and this PR can be merged even while breaking darwin.

I'll remove my request changes to reflect that.

@uri-canva uri-canva self-requested a review September 19, 2022 23:46
@winterqt
Copy link
Member

We should probably mark it as broken on Darwin before merging.

@fabianhjr
Copy link
Member Author

Marked as broken on darwin

@drupol
Copy link
Contributor

drupol commented Sep 20, 2022

Who's saying they're... gatekeepers? Seems like a very odd word choice (that has weird implications with it).

If it'a a simple fix, there's no reason it can't be added to this PR. (Note: I have no clue if it is.)
And if it's not, then it's fine -- just make sure to mark it as broken on Darwin (ideally).

Oops... I apologize for the word I used, I didn't know it could be misinterpreted, I sincerely hope I haven't offended anyone.

What I merely meant here is that if the Darwin team do not reply to solicitations, then we shouldn't wait and move on. I also had the same kind of issue and never got any single reply from the team, so I moved on. This is the reason why I thought that gatekeeper was appropriate, wrongly.

I think marking the package as broken for Darwin is a better idea indeed.

Hope no hard feelings on this and thanks for the quick reaction on this.

@winterqt
Copy link
Member

winterqt commented Sep 20, 2022 •
edited

What I merely meant here is that if the Darwin team do not reply to solicitations, then we shouldn't wait and move on.

Agreed -- I didn't realize the call was sent out almost 2 months ago, and that's my fault. Sorry!

@uri-canva uri-canva merged commit 9a3b426 into NixOS:master Sep 20, 2022
@uri-canva
Copy link
Contributor

Thanks for the patience everyone, sorry for causing some confusion!

@fabianhjr
Copy link
Member Author

No worries, hopefully someone with access to the darwin platform can sort the pending issue out.

@fabianhjr fabianhjr deleted the update-idea-community branch September 20, 2022 04:21
@drupol
Copy link
Contributor

drupol commented Sep 20, 2022

Thanks 🙏

@wamserma wamserma mentioned this pull request Sep 20, 2022
Closed
13 tasks
@fabianhjr fabianhjr mentioned this pull request Mar 10, 2023
Merged
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mindavi Mindavi Mindavi approved these changes

@drupol drupol drupol approved these changes

@khrj khrj khrj approved these changes

@edwtjo edwtjo Awaiting requested review from edwtjo

@azahi azahi Awaiting requested review from azahi

@AnatolyPopov AnatolyPopov Awaiting requested review from AnatolyPopov

@thefloweringash thefloweringash Awaiting requested review from thefloweringash

@uri-canva uri-canva Awaiting requested review from uri-canva

Assignees
No one assigned
Labels
8.has: package (new) 10.rebuild-darwin: 1-10 10.rebuild-linux: 501-1000 10.rebuild-linux: 501+
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
10 participants
@fabianhjr @AnatolyPopov @drupol @Mindavi @veprbl @winterqt @uri-canva @thefloweringash @khrj @bobby285271