Skip to content

kmscon: unstable-2018-09-07 -> 9.0.0#178749

Merged
AndersonTorres merged 1 commit intoNixOS:masterfrom
Infinidoge:bump/kmscon
Aug 27, 2022
Merged

kmscon: unstable-2018-09-07 -> 9.0.0#178749
AndersonTorres merged 1 commit intoNixOS:masterfrom
Infinidoge:bump/kmscon

Conversation

@Infinidoge
Copy link
Contributor

@Infinidoge Infinidoge commented Jun 23, 2022

Description of changes

This updates kmscon to the latest release tag on the repository, after 3 years(!) without an update in Nixpkgs.

NOTE: This PR depends on #171069

Notifying maintainer; @omasanori

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.

@Infinidoge
Copy link
Contributor Author

Infinidoge commented Jun 23, 2022

As noted in #178748, updating meson appears to have created a small variety of build errors due to the fortify hardening. It seems that meson is removing the -O2 flag from the hardening, causing the compiler to reject it due to the fortify hardening requiring the optimization flag.

This also occurred in the wayland-protocols package while building, which also uses meson.

@ofborg ofborg bot requested review from AndersonTorres, jtojnar and omasanori June 23, 2022 16:10
@ofborg ofborg bot added 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. labels Jun 23, 2022
@Infinidoge
Copy link
Contributor Author

Was made aware that someone else made a meson PR, so retargetting this on top of that.

@Infinidoge Infinidoge added the 2.status: blocked by pr/issue Another PR or issue is preventing this from being completed label Jun 23, 2022
@ofborg ofborg bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Jun 23, 2022
@Infinidoge
Copy link
Contributor Author

Infinidoge commented Jun 23, 2022

Rebased on top of the meson changes and edited the PR message accordingly.

I didn't see anything in the Nixpkgs manual about the case of depending on another PR being merged, so my (current) assumption is that when the meson change is merged, the commits this PR has will be seen as already included and thus won't have duplicates, but I could very much be wrong.

Please let me know if I should just rebase out the commits this depends on.

Also since this update by itself shouldn't cause a mass rebuild, it still targets master.

@omasanori
Copy link
Contributor

Thank you! I will check the changes in few days.

Copy link
Contributor

@omasanori omasanori left a comment

Choose a reason for hiding this comment

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

Regarding Meson changes, I think it would be better to keep them out of this PR and wait for the separate PR to be merged.

@Infinidoge
Copy link
Contributor Author

Rebased out the dependent commits.

@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Jun 26, 2022
@ofborg ofborg bot requested a review from omasanori June 26, 2022 23:23
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. and removed 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. labels Jun 26, 2022
@Infinidoge Infinidoge force-pushed the bump/kmscon branch 2 times, most recently from 2c3d08f to 67d5512 Compare June 26, 2022 23:56
@Infinidoge
Copy link
Contributor Author

Incidentally included the auto-formatting my editor did for lines 1/2, 10/11, and 33/34, which I'm going to leave in since I personally prefer the expanded form. If I should revert that shift, please let me know.

@omasanori
Copy link
Contributor

Incidentally included the auto-formatting my editor did for lines 1/2, 10/11, and 33/34, which I'm going to leave in since I personally prefer the expanded form. If I should revert that shift, please let me know.

It is acceptable to me.

By the way, what kind of tests have you done? Just completed a build or running the kmscon service with this? While I will do some tests this weekend anyways, I'd know what is succeeded on your machine.

@Infinidoge
Copy link
Contributor Author

Infinidoge commented Jun 30, 2022

While I haven't done so yet, I'll be swapping out my version to the updated one later today. (I had actually been tracking with the kmscon master branch up until the meson change, since I didn't change the build system at the time. Now seemed like a good time.)

The main testing I've been doing is checking out the command line arguments, and making sure it builds.

@Infinidoge
Copy link
Contributor Author

For those who want to more easily test the changes using flakes, I've made a branch on my nixpkgs fork that combines the meson and kmscon changes: https://github.com/Infinidoge/nixpkgs/tree/combined/kmscon
(github:Infinidoge/nixpkgs/combined/kmscon)

@Infinidoge
Copy link
Contributor Author

Seems to be running just fine on my machine, which is good:
image

Since the relative feature changes were small, it's a bit hard to test them specifically, however I am setting up a compose key to try using that under kmscon.

Copy link
Member

@AndersonTorres AndersonTorres left a comment

Choose a reason for hiding this comment

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

Waiting for the next meson.

@bobby285271 bobby285271 added the 12.approvals: 1 This PR was reviewed and approved by one person. label Jul 2, 2022
Copy link
Contributor

@omasanori omasanori left a comment

Choose a reason for hiding this comment

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

While I have still been building the system on my old laptop, I reviewed changes between two commits briefly and finds no huge and risky changes.

Feel free to merge this when meson is updated even if I have not yet reported the result on my machine at that moment. I will test as soon as possible though.

@bobby285271 bobby285271 added 12.approvals: 2 This PR was reviewed and approved by two persons. 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in any of the changed packages. and removed 12.approvals: 1 This PR was reviewed and approved by one person. labels Jul 4, 2022
@omasanori
Copy link
Contributor

I had tested underline, italic and multilingual text with emacs-nox, noto-fonts and noto-fonts-cjk-sans and confirmed that it works. Thanks @Infinidoge !


# _FORTIFY_SOURCE requires compiling with optimization (-O)
NIX_CFLAGS_COMPILE = lib.optionalString stdenv.cc.isGNU "-O"
+ " -Wno-error=maybe-uninitialized"; # https://github.com/Aetf/kmscon/issues/49
Copy link
Contributor

Choose a reason for hiding this comment

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

In my opinion, downstream packagers should never compile with Werror, since it can break on compiler updates. That's ubdesirable since a compiler update (generally) won't make the source code less safe in any way. And the compiler shouldn't be assumed anyway since different distros may use different compilers.

Copy link
Contributor

Choose a reason for hiding this comment

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

So my suggestion is to add a blanket -Wno-error (or set a flag that does that)

@superherointj superherointj changed the title kmscon: unstable-2018-09-07 -> v9.0.0 kmscon: unstable-2018-09-07 -> 9.0.0 Aug 2, 2022
@bobby285271 bobby285271 added 12.approvals: 3+ This PR was reviewed and approved by three or more persons. and removed 12.approvals: 2 This PR was reviewed and approved by two persons. labels Aug 14, 2022
@Infinidoge
Copy link
Contributor Author

Since the dependent PR has been merged, is there anything that needs to be done before this can be merged?

@Mindavi
Copy link
Contributor

Mindavi commented Aug 25, 2022

@ofborg eval

@ofborg ofborg bot requested a review from omasanori August 25, 2022 08:04
@Infinidoge
Copy link
Contributor Author

Also, it seems that the meson changes have made their way into master, should I go ahead and retarget to there, since this change itself doesn't cause a mass rebuild?

@Infinidoge
Copy link
Contributor Author

Oh wait it already is targetting master, nevermind

@AndersonTorres AndersonTorres merged commit f6677f4 into NixOS:master Aug 27, 2022
@Infinidoge Infinidoge deleted the bump/kmscon branch August 27, 2022 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

2.status: blocked by pr/issue Another PR or issue is preventing this from being completed 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. 12.approvals: 3+ This PR was reviewed and approved by three or more persons. 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in any of the changed packages.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants