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

From-source nix-built artifacts without the "nix" Extra Attribute #31

Merged
merged 7 commits into from
Oct 9, 2023

Conversation

roberth
Copy link
Collaborator

@roberth roberth commented Oct 5, 2023

No description provided.

Copy link
Contributor

@raboof raboof left a comment

Choose a reason for hiding this comment

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

one small thing to fix on the README, otherwise LGTM, great improvements

CONTRIBUTING.md Show resolved Hide resolved
README.md Outdated
@@ -155,6 +155,13 @@ sbtix.buildSbtProgram {
}
```

#### `nix` Extra Attribute

Source dependencies do not need locking. Originally this was implemented by completely ignoring dependencies with `nix` [extra attribute](https://www.scala-sbt.org/1.x/docs/Library-Management.html#Extra+Attributes), but this is not necessary anymore.
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be confusing to document how it 'originally' worked, maybe just leave that out?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It does still exist, which is what I wanted to document.
I've reduced it to more of a statement of fact.
Maybe my approach is too conservative and we should rip it out?


nix Extra Attribute

By adding the nix extra attribute, sbtix will ignore the dependency for the purpose of locking.

This used to be the only mechanism for handling local dependencies, but is now a legacy solution and/or escape hatch.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah right. I guess a reason to keep it would be so you could disambiguate between a nix-built dependency and the same version from a repo somewhere? I don't feel strongly either way.

# This is unfortunate, because it will create duplication in the store during builds and development,
# but at least these can be GC-ed and allow the end result to only reference the single JARs that
# result from this copying operation.
copyFile (baseNameOf urlAttrs.path) (localBuildsRepo + "/" + urlAttrs.path)
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I see how this is an improvement in some cases, though indeed not in others. I'm OK with leaving it like this for now and perhaps fine-tuning further in the future.

plugin/nix-exprs/sbtix.nix Show resolved Hide resolved
tests/multi-build/three/build.sbt Show resolved Hide resolved
(cherry picked from commit 3c43b5e)
Focus less on what was.

This section should be moved into reference docs that aren't the README.
It is not at all essential.
It wasn't used by the Nix code.
Copy link
Contributor

@raboof raboof left a comment

Choose a reason for hiding this comment

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

(the repo is available on the sbt side under two names, to support both maven and ivy style)

src/sbtix.sh Outdated Show resolved Hide resolved
Co-authored-by: Arnout Engelen <arnout@bzzt.net>
hashBash =
if urlAttrs.type or null == "built"
then ''$(sha256sum "${artifact}" | cut -c -64)''
else ''$(echo ${toLower urlAttrs.sha256} | tr / _)'';
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@raboof We could use sha256sum as an alternative here too, until the hash conversion functions are standardized. Does have a performance cost though.

Refs #20 (comment)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Possible hash format conversion primops

Copy link
Contributor

Choose a reason for hiding this comment

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

using the urlAttrs.sha256 seems OK to me

@Zhen-hao Zhen-hao merged commit 26daf96 into master Oct 9, 2023
2 checks passed
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