sass / dart-sass

The reference implementation of Sass, written in Dart.
https://sass-lang.com/dart-sass
MIT License
3.9k stars 352 forks source link

check in the dart pub lockfile in tree #2157

Closed selfisekai closed 8 months ago

selfisekai commented 8 months ago

this package provides an executable, and as such should contain the lockfile

ntkme commented 8 months ago

https://github.com/sass/dart-sass/pull/1948#issuecomment-1539185332

selfisekai commented 8 months ago

that goes against distro policies that are in place so we know what we're actually building and shipping to users. lockfiles include checksums of the dependencies used

ntkme commented 8 months ago

I don't think check this in is the right solution, as it adds more problem to development and testing.

A better solution would be to add a pubspec.lock as release artifact. The release procedure would be:

  1. Generate a pubspec.lock, upload as the action artifact.
  2. Download from action artifact, build for each architecture with the same pubspec.lock.
  3. Upload pubspec.lock as a release artifact.
nex3 commented 8 months ago

@selfisekai What's the specific goal of having a pubspec.lock file? Is it to ensure you're using some notion of "the same" dependency versions as a given Dart Sass release? Because not all installation methods use those same versions—anyone installing via pub or Homebrew are only guaranteed a compatible dependency constellation, per semver. Nor do we do any particular verification of the checksums—we just trust the GitHub Actions VM's HTTPS connection to pub.dev, so I'm not sure they provide any particular value beyond the checksums you'd get by constructing a pubspec.lock manually.

If this is still very important, I suppose there's no harm in uploading a lockfile along with the release, but I'm not sure it'll actually provide any material benefits beyond what you'd get by creating one yourself.

selfisekai commented 8 months ago

What's the specific goal of having a pubspec.lock file?

reproducible builds (versions and checksums pinned, so builds are not changed by dependency's new release or a swap). from the most practical side tho, we have to rebuild packages regularly (e.g. with new dart release). a lockfile just limits the possibilities of other, possibly breaking changes

so I'm not sure they provide any particular value beyond the checksums you'd get by constructing a pubspec.lock manually.

not separately maintaining a >600 lines file that has to be updated on version bumps

ntkme commented 8 months ago

To provide a bit more context. @selfisekai a maintainer of alpine-linux. They have they own packaging of dart-sdk and dart-sass that is different from https://github.com/dart-musl/dart and the linux-musl package we ship.

This is https://github.com/dart-musl/dart:

/ # ldd /usr/lib/dart/bin/dart
    /lib/ld-musl-aarch64.so.1 (0xffff8b6f0000)
    libc.musl-aarch64.so.1 => /lib/ld-musl-aarch64.so.1 (0xffff8b6f0000)

This is dart-sdk from alpine-linux:

/ # ldd /usr/bin/dart
    /lib/ld-musl-aarch64.so.1 (0xffffac9f9000)
    libz.so.1 => /lib/libz.so.1 (0xffffaa53e000)
    libicuuc.so.74 => /usr/lib/libicuuc.so.74 (0xffffaa34b000)
    libstdc++.so.6 => /usr/lib/libstdc++.so.6 (0xffffaa0a6000)
    libgcc_s.so.1 => /usr/lib/libgcc_s.so.1 (0xffffaa075000)
    libc.musl-aarch64.so.1 => /lib/ld-musl-aarch64.so.1 (0xffffac9f9000)
    libicudata.so.74 => /usr/lib/libicudata.so.74 (0xffffaa055000)

The main difference is that https://github.com/dart-musl/dart is optimized for compatibility and portability that all libraries except musl-libc are bundled. It is the same way like the official dart-sdk that all libraries except glibc are bundled.

The dart-sdk shipped with alpine-linux is optimized for binary size, that it is dynamically linked with dependencies libraries. The dart-sass package that alpine-linux ships is also optimized for binary size, that the package is effectively only an aot-snapshot and a shell script. The dart-sass package from alpine-linux does not ship any dart runtime by itself, as the shell script just runs the dart runtime provided by the dart-sdk package. - Therefore if the dart-sdk package gets an update, the dart-sass package would need a rebuild, because aot-snapshot is sdk version dependent. - I think what @selfisekai is looking for is that each time dart-sass is rebuild for a new sdk version it would get the same dependency.

ntkme commented 8 months ago

@selfisekai Question for you is that, what's the issue with the approach that you already have pubspec.lock checked-in in aports?

https://git.alpinelinux.org/aports/tree/testing/dart-sass/lock.patch

This should already give you “reproducible” builds. If your goal is to provide a build that is same as what we've released here in this repo, I have to remind you we don't rebuild dart-sass release when new dart-sdk is available, so even if you have pubspec.lock it is not going to be the same (there is no guarantee that new sdks won't break things).

nex3 commented 8 months ago

Yeah, that's the thing—I don't think checking in pubspec.lock (or uploading it as a build artifact) materially makes builds any more "reproducible" than storing a pubspec.lock yourself. It would allow you to use the same constellation of dependencies that we use for some of our releases of a given version of Dart Sass, but even that is contingent (in that we don't necessarily use those dependencies for pub or Homebrew releases) and coincidental (in that we don't consider that set of dependencies canonical and they undergo no special vetting).

selfisekai commented 8 months ago

it does not, it's just about the fact of maintaining it separately

nex3 commented 8 months ago

If this is just for convenience, I'd prefer to avoid checking in the lockfile.