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

nodejs-11_x: remove #70256

Merged
merged 3 commits into from Oct 4, 2019
Merged

nodejs-11_x: remove #70256

merged 3 commits into from Oct 4, 2019

Conversation

Ma27
Copy link
Member

@Ma27 Ma27 commented Oct 2, 2019 •
edited

Motivation for this change

Drops NodeJS v11 as discussed in #69008.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @

@Ma27 Ma27 added the 9.needs: port to stable A PR needs a backport to the stable release. label Oct 2, 2019
@Ma27 Ma27 added this to the 19.09 milestone Oct 2, 2019
@Ma27 Ma27 requested a review from FRidh as a code owner October 2, 2019 15:16
eadwu
eadwu reviewed Oct 2, 2019
View reviewed changes
pkgs/applications/version-management/sourcehut/core.nix
@@ -16,7 +16,7 @@ buildPythonPackage rec {

node_modules = fetchNodeModules {
src = "${src}/srht";
nodejs = nodejs-11_x;
nodejs = nodejs;
sha256 = "0axl50swhcw8llq8z2icwr4nkr5qsw2riih0a040f9wx4xiw4p6p";
Copy link
Member

Choose a reason for hiding this comment

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

The hash probably changes with different Node.js versions so it would be better to pin it to a certain release and change it everytime the version reaches EOL.

Copy link
Member

Choose a reason for hiding this comment

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

Nevermind it doesn't seem to have changed.

This was referenced Oct 2, 2019
Closed
Merged
@ofborg ofborg bot requested a review from eadwu October 2, 2019 15:29
@Ma27
Copy link
Member Author

Ma27 commented Oct 3, 2019

Will fix the merge conflict later that day :)

@Ma27
python3Packages.asyncpg: fix hash
7c74ebd 
The hash to the patch is broken, even with the original revision
which adds asyncpg (ee2161c). As the
downloaded patch seems fine, I guess that it was generated with
`nix-prefetch-url` (the hashes for `fetchpatch` usually differ) and the
issue wasn't found as the fixed-output-derivation was already in the
contributor's store.

See https://hydra.nixos.org/build/102495795

ZHF NixOS#68361
@Ma27
nodejs-11_x: remove
334641d 
Package is EOLed by upstream: https://github.com/nodejs/Release

Fixes NixOS#69008
@Ma27
sourcehut: mark as broken
594378c 
There were several custom python dependencies broken. I decided to
modify the `sourcehut` expression as it wouldn't even evaluate without
nodejs-11_x I didn't manage to get it building.
@Ma27
Copy link
Member Author

Ma27 commented Oct 3, 2019

Rebased onto latest master and resolved conflicts in the release notes.

@eadwu eadwu mentioned this pull request Oct 3, 2019
Closed
10 tasks
jonringer
jonringer approved these changes Oct 3, 2019
View reviewed changes
Copy link
Contributor

@jonringer jonringer left a comment

Choose a reason for hiding this comment

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

nix-review looks right

[1 built, 0.0 MiB DL]
https://github.com/NixOS/nixpkgs/pull/70256
9 package are marked as broken and were skipped:
sourcehut.buildsrht sourcehut.dispatchsrht sourcehut.gitsrht sourcehut.hgsrht sourcehut.listssrht sourcehut.mansrht sourcehut.metasrht sourcehut.pastesrht sourcehut.todosrht

1 package were build:
python37Packages.asyncpg

@jonringer
Copy link
Contributor

@GrahamcOfBorg build python3Packages.asyncpg

@Ma27
Copy link
Member Author

Ma27 commented Oct 4, 2019

@jonringer anything missing here? Otherwise this should be good to go, right? :)

@gilligan
Copy link
Contributor

gilligan commented Oct 4, 2019

LGTM too ;)

andir
andir reviewed Oct 4, 2019
View reviewed changes
nixos/doc/manual/release-notes/rl-1909.xml
@@ -519,6 +519,11 @@
Please use the fork <literal>cawbird</literal> instead which has been adapted to the API changes and is still maintained.
</para>
</listitem>
<listitem>
<para>
The <literal>nodejs-11_x</literal> package has been removed as it's EOLed by upstream.
Copy link
Member

Choose a reason for hiding this comment

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

This has to be updated for master. We should be targeting 20.03 now.

Copy link
Member Author

Choose a reason for hiding this comment

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

So in your opinion this shouldn't land in 19.09? The thing is, NodeJS 11 is actually EOLed (see also #69008).

Copy link
Member

Choose a reason for hiding this comment

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

No, entirely not my intention.

I just think we should deprecate on master first and then backport it to whever we want it to be. If those target branches then are an upcoming release those should receive an appropriate changelog entry.

If we would merge this now the changelog (from a master perspective) would not reflect the reality of 19.09. That is why I am saying we should update this PR to 20.03. After this has been accepted into 19.09 (by the RMs) then we should go ahead and remove the changelog entry from 20.03 since it would then no longer be true.

We could also drop the message here and only add it to the backport PR. I do not like that since we do not know ahead of time if the RMs would like this kind of change this late in the process.

That all being said. I think our RMs are very reasonable and will happily accept this even at this time.

Copy link
Member

Choose a reason for hiding this comment

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

+1 for backporting right away, it sucks but shipping EOL software sucks more :)

Copy link
Member Author

Choose a reason for hiding this comment

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

@andir as we should backport this to 19.09, I guess it should be fine to keep this in the 19.09 release notes, right? (Just talked to @lheckemann about this in person, so unless there are any further objections, I guess it should be fine to merge :))

Copy link
Contributor

Choose a reason for hiding this comment

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

sigh, I wish I had nixos users to talk to :(. Sad times in the states :/

@lheckemann lheckemann merged commit 141b721 into NixOS:master Oct 4, 2019
@Ma27 Ma27 deleted the drop-nodejs-11_x branch October 4, 2019 16:18
@Ma27
Copy link
Member Author

Ma27 commented Oct 4, 2019

Backported as 28a0cae, 3a12434 and 4e3230f.

@Ma27 Ma27 added 8.has: port to stable A PR already has a backport to the stable release. and removed 9.needs: port to stable A PR needs a backport to the stable release. labels Oct 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lheckemann lheckemann lheckemann left review comments

@andir andir andir left review comments

@jonringer jonringer jonringer approved these changes

@gilligan gilligan gilligan approved these changes

@marsam marsam marsam approved these changes

@disassembler disassembler Awaiting requested review from disassembler

@FRidh FRidh Awaiting requested review from FRidh

@eadwu eadwu Awaiting requested review from eadwu

Assignees
No one assigned
Labels
6.topic: nixos 6.topic: python 8.has: changelog 8.has: clean-up 8.has: documentation 8.has: port to stable A PR already has a backport to the stable release. 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10
Projects
None yet
Milestone
19.09
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@Ma27 @jonringer @gilligan @marsam @lheckemann @andir @eadwu