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

check in the dart pub lockfile in tree #2157

Closed
wants to merge 1 commit into from

Conversation

selfisekai
Copy link

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

this package provides an executable, and as such should contain
the lockfile
@ntkme
Copy link
Contributor

ntkme commented Dec 30, 2023

#1948 (comment)

@selfisekai
Copy link
Author

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
Copy link
Contributor

ntkme commented Dec 30, 2023

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
Copy link
Contributor

nex3 commented Jan 2, 2024

@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.

@nex3 nex3 added the needs info label Jan 2, 2024
@selfisekai
Copy link
Author

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
Copy link
Contributor

ntkme commented Jan 3, 2024

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
Copy link
Contributor

ntkme commented Jan 3, 2024

@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
Copy link
Contributor

nex3 commented Jan 3, 2024

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
Copy link
Author

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

@nex3
Copy link
Contributor

nex3 commented Jan 4, 2024

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

@nex3 nex3 closed this Jan 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants