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

Martin-ui packaging issue #1699

Merged
merged 20 commits into from
Mar 4, 2025
Merged

Martin-ui packaging issue #1699

merged 20 commits into from
Mar 4, 2025

Conversation

nyurik
Copy link
Member

@nyurik nyurik commented Feb 23, 2025

Seems we still have a packaging issue - npm saves files outside the build dir.

error: failed to verify package tarball

Caused by:
  Source directory was modified by build.rs during cargo publish. Build scripts should not modify anything outside of OUT_DIR.
  Added: .../martin/target/package/martin-0.15.0/martin-ui/dist
  	.../martin/target/package/martin-0.15.0/martin-ui/dist/_
  	.../martin/target/package/martin-0.15.0/martin-ui/dist/_/assets
        ...

Fixes #1667
See prior partial fix in #1668

Seems we still have a packaging issue - npm saves files outside the build dir
@CommanderStorm
Copy link
Collaborator

CommanderStorm commented Feb 23, 2025

Let me prephase this: What I have just pushed is a hooorible hack.

What I just pushed works locally (both publish and running).. wish me luck

image

@CommanderStorm
Copy link
Collaborator

I will not continue working on this today, seems like CI is really against this change..
I need fresh eyes for this => will continue sometime in the week/on the weekend.

If someone wants to look into this or has better oppinions, I would love feedback.

Somewhere I apparently made a change in the code I pushed during final cleanup (=before my first commit here) 😞

@CommanderStorm CommanderStorm marked this pull request as draft February 23, 2025 23:13
@CommanderStorm CommanderStorm self-assigned this Mar 2, 2025
@CommanderStorm CommanderStorm marked this pull request as ready for review March 2, 2025 11:39
Copy link
Collaborator

@CommanderStorm CommanderStorm left a comment

Choose a reason for hiding this comment

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

Okay.. this works now.
It is still a hoooorible hack, but at least it woks

nyurik and others added 3 commits March 3, 2025 23:20
Best to skip recursion when we can let a well known crate do the work
for us
@nyurik
Copy link
Member Author

nyurik commented Mar 4, 2025

thx, i refactored the build script a bit further, trying to simplify it (not much luck), plus made sure not to use results - with build, its ok to crash asap as it gives us good debugging context. When we build with the npm packaging command, where does the generated file end up in? I only see dist/index.html.

If everything is ok, lets merge

@CommanderStorm
Copy link
Collaborator

When we build with the npm packaging command, where does the generated file end up in? I only see dist/index.html.

what do you mean? Not quite sure what you are asking
It is in the OUT_DIR, more precicely most of from the node build generated files will end up in the _ folder.
image

Copy link
Collaborator

@CommanderStorm CommanderStorm left a comment

Choose a reason for hiding this comment

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

Looks good to me
Could be merged as is, asssuming rerun-if-changed is correct ^^

nyurik and others added 2 commits March 4, 2025 10:48
@nyurik
Copy link
Member Author

nyurik commented Mar 4, 2025

thanks! Turns out I shouldn't code at night - didn't notice the _ dir. Thank you for good suggestions, will merge shortly.

Copy link
Collaborator

@CommanderStorm CommanderStorm left a comment

Choose a reason for hiding this comment

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

LGTM
I think we can remove the TODO though

"the martin-ui/dist must either not exist or have been produced by previous builds"
);

// TODO: we may need to move index.html one level down per change_detection() docs
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you clairify what you mean here?
When I rebuild, this should be reproducible enough to depend on the input files and not on the output file..

As far as I see it, this is a non-issue..

Suggested change
// TODO: we may need to move index.html one level down per change_detection() docs

@nyurik nyurik merged commit 48ff2c1 into maplibre:main Mar 4, 2025
20 checks passed
@nyurik nyurik deleted the npm-fix branch March 4, 2025 17:37
@nyurik
Copy link
Member Author

nyurik commented Mar 4, 2025

Thanks @CommanderStorm for tackling this one! Messy, but it seems like it works now :)

This is why i put that todo (which we might want to remove later):

https://github.com/static-files-rs/static-files/blob/bac6352faddffdc36d96cac04b4115dfd6f2f35e/src/mods/npm_build.rs#L81-L93

@nyurik nyurik restored the npm-fix branch March 4, 2025 17:53
@nyurik nyurik deleted the npm-fix branch March 4, 2025 17:53
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.

cargo publish -p martin does not work due to Node.js package issue
2 participants