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

[v1.40.0] Remove code deprecated in v1.30.0 #2924

Draft
wants to merge 27 commits into
base: master
Choose a base branch
from

Conversation

Geod24
Copy link
Member

@Geod24 Geod24 commented Jun 5, 2024

Release v1.30.0 was a massive release (337 commits).

The main changes were:

  • Various API deprecation as Dependency was made a discriminated union and its various components were broken down;
  • Overrides were deprecated;
  • Configy was used to parse all JSON files;
  • Ability to set Dub's root path and a few other items were deprecated;

Once v1.39.0 is released, we can merge this PR and get rid of a lot of deprecated code, which will also massively simplify future work.

Copy link

github-actions bot commented Jun 5, 2024

BUILD FAILED
❌ Basic dub build failed! Please check your changes again.

Build statistics:

 statistics (-before, +after)
 executable size=5259464 bin/dub
-rough build time=64s
+rough build time=4s
Full build output
DUB version 1.37.0, built on May 11 2024
LDC - the LLVM D compiler (1.38.0):
  based on DMD v2.108.1 and LLVM 18.1.5
  built with LDC - the LLVM D compiler (1.38.0)
  Default target: x86_64-unknown-linux-gnu
  Host CPU: znver3
  http://dlang.org - http://wiki.dlang.org/LDC


  Registered Targets:
    aarch64     - AArch64 (little endian)
    aarch64_32  - AArch64 (little endian ILP32)
    aarch64_be  - AArch64 (big endian)
    amdgcn      - AMD GCN GPUs
    arm         - ARM
    arm64       - ARM64 (little endian)
    arm64_32    - ARM64 (little endian ILP32)
    armeb       - ARM (big endian)
    avr         - Atmel AVR Microcontroller
    bpf         - BPF (host endian)
    bpfeb       - BPF (big endian)
    bpfel       - BPF (little endian)
    hexagon     - Hexagon
    lanai       - Lanai
    loongarch32 - 32-bit LoongArch
    loongarch64 - 64-bit LoongArch
    mips        - MIPS (32-bit big endian)
    mips64      - MIPS (64-bit big endian)
    mips64el    - MIPS (64-bit little endian)
    mipsel      - MIPS (32-bit little endian)
    msp430      - MSP430 [experimental]
    nvptx       - NVIDIA PTX 32-bit
    nvptx64     - NVIDIA PTX 64-bit
    ppc32       - PowerPC 32
    ppc32le     - PowerPC 32 LE
    ppc64       - PowerPC 64
    ppc64le     - PowerPC 64 LE
    r600        - AMD GPUs HD2XXX-HD6XXX
    riscv32     - 32-bit RISC-V
    riscv64     - 64-bit RISC-V
    sparc       - Sparc
    sparcel     - Sparc LE
    sparcv9     - Sparc V9
    spirv       - SPIR-V Logical
    spirv32     - SPIR-V 32-bit
    spirv64     - SPIR-V 64-bit
    systemz     - SystemZ
    thumb       - Thumb
    thumbeb     - Thumb (big endian)
    ve          - VE
    wasm32      - WebAssembly 32-bit
    wasm64      - WebAssembly 64-bit
    x86         - 32-bit X86: Pentium-Pro and above
    x86-64      - 64-bit X86: EM64T and AMD64
    xcore       - XCore
   Upgrading project in /home/runner/work/dub/dub/
    Starting Performing "release" build using /opt/hostedtoolcache/dc/ldc2-1.38.0/x64/ldc2-1.38.0-linux-x86_64/bin/ldc2 for x86_64.
    Building dub 1.38.0+commit.71.gc38b74c2: building configuration [application]
source/dub/packagemanager.d(895,14): Error: no property `loadOverrides` for `repository` of type `dub.packagemanager.Location`
source/dub/packagemanager.d(1160,9):        struct `Location` defined here
Error /opt/hostedtoolcache/dc/ldc2-1.38.0/x64/ldc2-1.38.0-linux-x86_64/bin/ldc2 failed with exit code 1.
BUILD FAILED
STAT:statistics (-before, +after)
STAT:executable size=5259464 bin/dub
STAT:rough build time=4s

Copy link
Member

@s-ludwig s-ludwig left a comment

Choose a reason for hiding this comment

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

This will definitely require a 2.0.0 version bump!

There are a few removals that would be quite annoying for anyone using the original API. In general, as a user, I absolutely hate this kind of light-hearted process for cleaning up/changing APIs that prioritizes minimal improvements on the library side over the constant need to follow these changes throughout the user base (Android, looking at you!). But in this case, we can probably assume that the number of API users is low enough that this is not an issue.

@@ -528,9 +528,3 @@ enum Flags!BuildOption inheritedBuildOptions =
| BuildOption.ignoreDeprecations | BuildOption.deprecationWarnings
| BuildOption.deprecationErrors | BuildOption.property | BuildOption.profileGC
| BuildOption.pic;

deprecated("Use `Flags!BuildOption` instead")
public alias BuildOptions = Flags!BuildOption;
Copy link
Member

Choose a reason for hiding this comment

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

What is the value of deprecating/removing something like this? IMO, it makes code easier to read and also keeps a door open for backwards compatible adjustments of the exact type (exactly like what was happening in the mentioned original commit).

@Geod24
Copy link
Member Author

Geod24 commented Jun 5, 2024

This will definitely require a 2.0.0 version bump!

So we don't follow standard D policy of 10 releases ? It feels odd to have a stronger policy than the compiler. I get your point, and it's the correct thing to do, I don't know if it's the right one though.

In general, as a user, I absolutely hate this kind of light-hearted process for cleaning up/changing APIs that prioritizes minimal improvements on the library side over the constant need to follow these changes throughout the user base

I get that point, and we've definitely been guilty of doing this a bit too much in D. In the case of Dub, there was a lot of exploration needed to narrow down the scope of what the tool was doing. My favorite example was turning Dependency into a discriminated union: before that work, it was not possible to assess the fitness of an API. Dub.fetch(string, Dependency) was the most used API, but obviously it is too broad and that had to be reworked. Using the more user-friendly approach would probably have had an enormous upfront cost and not yielded as good a result. Note that I used what I consider a good middle ground: I did incremental changes locally, and when I was confident enough I was going in the right direction, I submitted a flurry of PR with small changes.

Note that if we want to cut a v2.0 release, I can prepare that, but I would like some more changes in then.

@s-ludwig
Copy link
Member

s-ludwig commented Jun 5, 2024

So we don't follow standard D policy of 10 releases ? It feels odd to have a stronger policy than the compiler. I get your point, and it's the correct thing to do, I don't know if it's the right one though.

We can follow the same time line, just the version scheme is defined as SemVer and should be treated accordingly.

Using the more user-friendly approach would probably have had an enormous upfront cost and not yielded as good a result.

Yes, there is definitely a trade-off and many of the changes here are not problematic anyway.

Note that if we want to cut a v2.0 release, I can prepare that, but I would like some more changes in then.

Definitely makes sense to collect as many breaking changes as possible, as long as it doesn't add considerable amounts of additional development costs. I wouldn't necessarily treat this like you would treat a typical product version (i.e. the new shiny version two is out!), but simply as an incremental change that collects a bunch of breaking changes to avoid too frequent breakage.

@Geod24
Copy link
Member Author

Geod24 commented Jun 11, 2024

We can follow the same time line, just the version scheme is defined as SemVer and should be treated accordingly.

To me it doesn't really make sense though, as the release would not remove all the deprecated code, just some. So I'd rather delay things if that's the case (although removing overrides will simplify things quite a lot).

I wouldn't necessarily treat this like you would treat a typical product version (i.e. the new shiny version two is out!), but simply as an incremental change that collects a bunch of breaking changes to avoid too frequent breakage.

Another possible approach would be to start a v2 branch and rebase it from time to time until it's ready to be released. But since I'm the only person that would work on it, I can just keep it locally.

Geod24 added 20 commits June 11, 2024 18:08
The override system has been deprecated since v1.30.0 (52e3f86).
It has now been 10 releases, and following the standard Dlang policy,
we are clear to remove it.
This warning was added in 67a58e4
and the plan was to enable it in 10 releases (v1.40.0).
This warning was introduced in a8aa234.
@atilaneves
Copy link
Contributor

Removing deprecated code means changing the API - I agree that the version should be bumped to 2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants