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

$mol_build check mam submodule and do not init #727

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

zerkalica
Copy link
Collaborator

No description provided.

@zerkalica zerkalica enabled auto-merge January 15, 2025 22:18
@zerkalica zerkalica force-pushed the check-mam-submodule branch from 1c391d2 to baacb72 Compare January 16, 2025 10:58
return dirs.some(str => str && dir.endsWith('/' + str))
} catch (e) {
if ($mol_promise_like(e)) $mol_fail_hidden(e)
console.error(e)
Copy link
Member

Choose a reason for hiding this comment

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

А чего не $mol_fail_log?

try {
const dirs = this.submodule_dirs({ dir: parent })

return dirs.some(str => str && dir.endsWith('/' + str))
Copy link
Member

@nin-jin nin-jin Jan 16, 2025

Choose a reason for hiding this comment

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

Грязная проверка, может случайно совпасть не с тем. лучше явно отрезолвить абсолютный путь.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

а как?

const dir = this.root().path()
if (! this.is_git( dir ) ) return new Set<string>()

const dirs = this.submodule_dirs({ dir, recursive: true })
.map(str => `${dir}/${str}`)
Copy link
Member

Choose a reason for hiding this comment

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

Есть же апишка для резолвинга путей, к чему тут ручное склеивание?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

а зачем усложнять, тут же str - 100% кусок пути относительно dir

Copy link
Member

@nin-jin nin-jin Jan 16, 2025

Choose a reason for hiding this comment

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

dir.resolve(str)
Но я бы лучше сразу в submodule_dirs резолвил.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

как то это все усложняет код с виду, тогда надо dir передавать не строкой, а объектом, либо в submodule_dirs mol_file.absolute вызывать

либо вообще все на объекты переделывать, но мало где нужен именно $mol_file, со строкой проще отлаживать

@@ -43,6 +43,7 @@ namespace $ {
@ $mol_action
protected update_safe(dir: string) {
if (this.update_disabled) return false
if ( this.$.$mol_env()['MOL_BUILD_VSC_UPDATE_DISABLE'] ) return false
Copy link
Member

Choose a reason for hiding this comment

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

Вот так вот поотключают обновления, а потом мучаются с устаревшим кодом.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ну с докером по-другому не сделать воспроизводимых билдов, а все хотят docker compose up и получить на другой машине такой же билд, пусть отключат для CI, если очень надо.

Copy link
Member

@nin-jin nin-jin Jan 16, 2025

Choose a reason for hiding this comment

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

Зачем фронтенд каждый раз собирать из исходников? Один раз собрали, проверили, положили в контейнер и поднимай в докере этот снепшот хоть до скончания времён.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

это не будет работать, если одну строчку надо поправить, конфиг какой-нить, а все новое прилетит

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.

2 participants